chore: filter attestations by hash and payload#145
Conversation
WalkthroughExtended listAttestations to accept optional byte filters Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SDK (listAttestations)
participant ContractView as Contract View
rect rgba(60,179,113,0.08)
note right of SDK: new optional byte filters
end
Client->>SDK: listAttestations(filters: requester?, requestTxId?, attestationHash?, resultCanonical?, ...)
SDK->>ContractView: viewListAttestations({ requester, request_tx_id, attestation_hash, result_canonical, ... })
ContractView-->>SDK: attestations[]
SDK-->>Client: attestations[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types/attestation.ts (1)
480-494: Unit tests look good, but consider adding size validation.The tests properly verify that the validation function accepts the new filter fields. However, unlike
requester(which validates exactly 20 bytes at line 280-282), there's no size validation forattestationHashorresultCanonical.If you want to add defensive validation (optional), consider:
// Validate requester length if (input.requester !== undefined) { if (input.requester.length !== 20) { throw new Error('requester must be exactly 20 bytes'); } } + + // Validate attestationHash length (if you want strict validation) + if (input.attestationHash !== undefined) { + if (input.attestationHash.length !== 32) { + throw new Error('attestationHash must be exactly 32 bytes'); + } + } + + // Note: resultCanonical can be variable length, so no validation needed // Validate orderByThis is optional since the backend likely handles invalid sizes gracefully, but it provides earlier error detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/contracts-api/attestationAction.ts(2 hunks)src/types/attestation.ts(3 hunks)tests/integration/attestation.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/attestation.test.ts (1)
tests/integration/utils.ts (1)
testWithDefaultWallet(9-11)
⏰ 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: test
🔇 Additional comments (5)
src/contracts-api/attestationAction.ts (2)
219-220: LGTM! Documentation accurately reflects the new filters.The documentation has been properly updated to mention filtering by attestation hash and result canonical bytes.
257-258: LGTM! Parameter handling is consistent.The new filter parameters follow the established pattern of using
new Uint8Array(0)to represent null BYTEA values, which is consistent with therequesterparameter handling.tests/integration/attestation.test.ts (1)
412-447: LGTM! Attestation hash filtering test is well-implemented.The test properly validates the new attestation hash filter by:
- Retrieving an existing attestation to obtain a valid hash
- Filtering attestations by that hash
- Verifying all returned results match the filter criteria
src/types/attestation.ts (2)
103-113: LGTM! New filter fields are properly documented.The new optional filter fields are well-defined with clear documentation and appropriate types.
466-468: LGTM! Test data properly covers the new fields.The test data has been updated to include the new filter parameters, ensuring validation works correctly with all fields present.
Time Submission Status
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/attestation.test.ts (1)
449-511: Tighten resultCanonical test so it always asserts the filter behaviorThis test now correctly derives
resultCanonicalfrom the signed payload and uses it inlistAttestations({ resultCanonical }), which addresses the earlier gap. However, if a signed payload is never obtained within the loop, theelsebranch only logs and the test exits without any assertion, so the filter behavior might never actually be validated while the test still passes.To ensure the test always verifies the filter when the feature is broken, you can fail fast if a valid signed payload isn’t obtained, and drop the silent “skip” path. For example:
- let signed = null; + let signed = null; for (let i = 0; i < 10; i++) { try { signed = await attestationAction.getSignedAttestation({ requestTxId: requestResult.requestTxId, }); if (signed.payload && signed.payload.length > 65) break; } catch (e) { await new Promise((resolve) => setTimeout(resolve, 2000)); } } - if (signed && signed.payload) { + // Ensure we actually obtained a canonical+signature payload + expect(signed && signed.payload && signed.payload.length > 65).toBe(true); + + if (signed && signed.payload && signed.payload.length > 65) { // Extract result_canonical from payload (payload = canonical + signature) // The canonical portion is everything except the last 65 bytes const resultCanonical = signed.payload.slice(0, -65); @@ - console.log(`Result canonical filter test completed: found ${filtered.length} attestation(s)`); - } else { - console.log("Attestation not signed yet, skipping result_canonical filter test"); - } + console.log(`Result canonical filter test completed: found ${filtered.length} attestation(s)`); + }This keeps the polling logic but guarantees the test will fail (and therefore actually protect the feature) if a valid signed attestation is never produced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/attestation.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/attestation.test.ts (1)
tests/integration/utils.ts (1)
testWithDefaultWallet(9-11)
⏰ 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: test
🔇 Additional comments (1)
tests/integration/attestation.test.ts (1)
412-447: Attestation-hash filter test is well structured and verifies behavior end-to-endThis test exercises the
attestationHashfilter in a realistic way: it derives a concrete hash from an existing attestation, re-queries with that hash, and then asserts that all returned attestations share exactly the same hash. That gives solid coverage of the new filter behavior and handles the “no attestations yet” case gracefully with logging.
2 participants
resolves: https://github.com/trufnetwork/trufscan/issues/141
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.