chore: compatibilize evm attestation integration#1218
Conversation
- Replaced the buildCanonical function with BuildCanonicalPayload, ensuring consistent use of big-endian length prefixes for variable sections. - Updated tests to reflect changes in the canonical payload structure, including adjustments to dataProvider and streamID. - Enhanced validation for canonical payloads to ensure compliance with expected formats, particularly for algorithm and length checks. - Introduced new helper functions for length-prefixing data, improving code clarity and maintainability. These changes enhance the reliability and consistency of the attestation processing workflow, ensuring accurate handling of canonical payloads.
WalkthroughAdds exported BuildCanonicalPayload and CanonicalPayload.ValidateForEVM; switches canonical length-prefixes to big-endian across payload building, hashing, precompiles, docs, and migrations; adds canonical→DataPoints ABI encoder and precompile; updates processor validation and hashing; introduces attestation E2E tests, fixtures, and tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Request / Tests
participant Builder as BuildCanonicalPayload
participant Parser as CanonicalPayload (parse)
participant Validator as ValidateForEVM
participant Hasher as computeAttestationHash
participant Signer as Signing Worker
Client->>Builder: (version, algo, height, provider, stream, action, args, result)
Builder-->>Client: canonical bytes (big-endian length-prefixed)
Client->>Parser: parse(canonical)
Parser->>Validator: ValidateForEVM()
Validator-->>Parser: ok / error
Parser->>Hasher: fields (with length-prefixes)
Hasher-->>Signer: attestation hash
Signer-->>Client: signature
Note over Builder,Validator: New: big-endian prefixes & EVM validation
sequenceDiagram
autonumber
participant SQL as request_attestation
participant Normalize as Migration logic
participant Precompile as canonical_to_datapoints_abi
participant Encoder as bytea_length_prefix (big-endian)
participant DB as Store attestation row
SQL->>Normalize: accept TEXT inputs ($data_provider, $stream_id)
Normalize->>Normalize: validate/normalize (hex, length)
Normalize->>Precompile: canonical_to_datapoints_abi($query_result)
Precompile-->>Normalize: result_payload (ABI)
Normalize->>Encoder: bytea_length_prefix(data_provider_bytes), bytea_length_prefix(stream_bytes)
Encoder-->>DB: store canonical payload + attestation_hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
- Updated the harness integration test to use byte arrays for dataProvider and streamID, ensuring consistency with EVM-compatible field sizes. - Added validation checks in the SQL migration to enforce specific byte lengths for dataProvider and streamID, improving data integrity. - Refactored end-to-end tests to implement a realistic stream-based workflow, enhancing the testing of the attestation process. These changes improve the reliability and maintainability of the attestation system, ensuring accurate processing and validation of attestations.
…nality - Replaced DROP and CREATE statements with CREATE OR REPLACE for the request_attestation and sign_attestation actions, enhancing the migration process. - Updated end-to-end tests to utilize clienttypes.WithSyncBroadcast for improved transaction handling and error management. - Enhanced error logging in the registerDataProvider function to provide clearer context on action availability. These changes improve the maintainability and reliability of the attestation actions and their associated tests, ensuring a more robust integration within the system.
- Changed data types for $data_provider and $stream_id in the request_attestation action from BYTEA to TEXT, improving input handling. - Added validation checks to ensure $data_provider is 0x-prefixed and has a length of 42 characters, and that $stream_id has a length of 32 characters, enhancing data integrity. - Updated end-to-end tests to reflect changes in data handling, ensuring consistency with the new input types and validation logic. These changes improve the robustness and reliability of the attestation process, ensuring accurate data validation and handling.
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/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 (2)
tests/extensions/tn_attestation/README.md (1)
40-54: Add language identifier to fenced code block.The test flow diagram would benefit from a language identifier for better syntax highlighting and parsing.
Apply this diff to add the identifier:
-``` +```text request_attestation (SQL) ↓ queue_for_signing (precompile adds to queue)Based on static analysis hints.
extensions/tn_utils/precompiles.go (1)
80-88: Update comment to reflect big-endian prefixingThe docstring still says “little endian,” but
lengthPrefixBytes(and the rest of the stack) now emit big-endian headers. Please refresh the wording so future readers don’t get tripped up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
extensions/tn_attestation/canonical.go(4 hunks)extensions/tn_attestation/canonical_test.go(3 hunks)extensions/tn_attestation/harness_integration_test.go(3 hunks)extensions/tn_attestation/integration_test.go(1 hunks)extensions/tn_attestation/processor.go(2 hunks)extensions/tn_attestation/processor_test.go(4 hunks)extensions/tn_utils/README.md(1 hunks)extensions/tn_utils/datapoints.go(1 hunks)extensions/tn_utils/precompiles.go(5 hunks)extensions/tn_utils/serialization_test.go(2 hunks)internal/migrations/024-attestation-actions.sql(4 hunks)tests/extensions/tn_attestation/README.md(1 hunks)tests/extensions/tn_attestation/attestation_e2e_test.go(1 hunks)tests/extensions/tn_attestation/docker-compose.yml(1 hunks)tests/extensions/tn_attestation/test_tn_attestation.sh(1 hunks)tests/streams/attestation/attestation_request_test.go(3 hunks)tests/streams/attestation/test_helpers.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T13:00:14.189Z
Learnt from: outerlook
PR: trufnetwork/node#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 (8)
extensions/tn_utils/precompiles.go (1)
extensions/tn_utils/datapoints.go (1)
EncodeDataPointsABI(32-64)
extensions/tn_attestation/processor_test.go (1)
extensions/tn_attestation/canonical.go (1)
BuildCanonicalPayload(38-58)
extensions/tn_utils/datapoints.go (1)
extensions/tn_utils/serialization.go (1)
DecodeQueryResultCanonical(188-239)
tests/extensions/tn_attestation/test_tn_attestation.sh (1)
tests/extensions/tn_attestation/attestation_e2e_test.go (1)
TestAttestationE2E(28-49)
extensions/tn_attestation/canonical_test.go (1)
extensions/tn_attestation/canonical.go (1)
BuildCanonicalPayload(38-58)
tests/extensions/tn_attestation/attestation_e2e_test.go (1)
extensions/tn_utils/serialization.go (1)
EncodeActionArgs(45-76)
extensions/tn_attestation/integration_test.go (1)
extensions/tn_attestation/canonical.go (1)
BuildCanonicalPayload(38-58)
tests/streams/attestation/attestation_request_test.go (3)
extensions/tn_utils/serialization.go (1)
EncodeQueryResultCanonical(145-184)extensions/tn_utils/datapoints.go (1)
EncodeDataPointsABI(32-64)extensions/tn_attestation/canonical.go (1)
BuildCanonicalPayload(38-58)
🪛 LanguageTool
tests/extensions/tn_attestation/README.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...tion-like environment. Complete flow: 1. Submit request_attestation → stores in...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...recompile → adds hash to in-memory queue 3. End-of-block hook → dequeues and process...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...ock hook → dequeues and processes hashes 4. Query database → finds unsigned attestat...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...y database → finds unsigned attestations 5. Sign → validator generates signature 6. ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... 5. Sign → validator generates signature 6. Broadcast → submits sign_attestation t...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...→ submits sign_attestation transaction 7. Retrieve → get_signed_attestation retu...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...Request confirmed and stored in database - ✅ Signature generated within 90 seconds ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ... 90 seconds (proves end-of-block worked) - ✅ Signed payload retrieved successfully ...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... ✅ Signed payload retrieved successfully - ✅ Payload structure valid: `canonical ||...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...ngle validator (no multi-node consensus) - Fixed validator key (deterministic for t...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...alidator key (deterministic for testing) - No network failure simulation - Assumes ...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...testing) - No network failure simulation - Assumes node is always leader ## Valida...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...istribute validator keys out of band and rotate them via node configuration, not ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
tests/extensions/tn_attestation/README.md
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (24)
tests/streams/attestation/test_helpers.go (4)
8-9: LGTM!The added imports support the new
mustHexToByteshelper function for hex string conversion.
27-28: LGTM!The test data constants have been updated to use deterministic hex patterns that align with EVM address formats, improving compatibility with the attestation encoding changes.
35-46: LGTM!The
mustHexToByteshelper correctly normalizes hex strings and panics on error, which is appropriate for package initialization. TheTestDataProviderBytesvariable provides convenient pre-decoded test data.
158-159: LGTM!Test helper methods correctly use the new hex-formatted test constants, maintaining consistency across the test suite.
Also applies to: 215-216
extensions/tn_utils/README.md (1)
16-16: Documentation correctly updated.The endianness change from little-endian to big-endian is clearly documented, aligning with the broader attestation encoding improvements.
extensions/tn_utils/serialization_test.go (2)
359-364: LGTM!Test expectations correctly updated to reflect big-endian 4-byte length prefixes. The encoding is accurate: length 2 →
[0, 0, 0, 2], length 3 →[0, 0, 0, 3].
401-403: LGTM!Multi-element prefix test expectations correctly updated for big-endian encoding:
[0, 0, 0, 1]for length 1 and[0, 0, 0, 2]for length 2.extensions/tn_attestation/processor.go (2)
142-144: LGTM!The addition of
ValidateForEVM()provides essential validation to ensure canonical payloads meet EVM compatibility requirements before signing. Good defensive programming.
218-219: LGTM!The hash computation correctly incorporates big-endian length prefixes for variable-length fields (
DataProvider,StreamID,Args), aligning with the new canonical payload structure. Note that this changes the hash calculation, so existing attestations will have different hashes under the new encoding.Also applies to: 224-224
extensions/tn_attestation/harness_integration_test.go (3)
6-6: LGTM!The
bytesimport is required for the new test data generation usingbytes.Repeat.
97-98: LGTM!Test data generation using
bytes.Repeatprovides deterministic byte patterns that better represent the actual data types (20-byte addresses, 32-byte IDs) used in production.
151-151: Algorithm value 0 is correct
All existing builders, parsers, and tests (processor_test.go, canonical_test.go, attestation_request_test.go) use algorithm = 0 andBuildCanonicalPayloadwrites the passed byte unconditionally, so the change is intentional and aligned with the canonical format.tests/extensions/tn_attestation/docker-compose.yml (3)
4-16: LGTM! Test-only security posture noted.The postgres configuration is appropriate for a test environment. The
POSTGRES_HOST_AUTH_METHOD=trustsetting allows passwordless connections, which is acceptable for isolated test containers but should never be used in production.
18-49: LGTM! Test configuration is comprehensive.The kwild service configuration properly depends on postgres health, uses appropriate test credentials, and includes comprehensive health checks. The private key is clearly a test key (sequential pattern) suitable only for test environments.
51-53: LGTM!The dedicated bridge network provides proper service isolation and internal name resolution for the test environment.
extensions/tn_utils/datapoints.go (5)
11-28: LGTM!Package initialization correctly sets up ABI argument types for
uint256[]andint256[]. The scale value of 18 matches standard Ethereum decimal precision. Panic on initialization failure is appropriate.
30-64: LGTM!The main ABI encoding function properly decodes canonical results, validates row structure (requiring at least 2 columns), and encodes to the expected ABI format with clear error messages.
66-84: LGTM!Timestamp extraction correctly handles both pointer and value types, validates non-NULL and non-negative constraints, and safely converts to
*big.Int.
86-98: LGTM!Data point value extraction correctly handles both pointer and value decimal types, validates non-NULL, and delegates to the scale conversion helper.
100-110: LGTM!Decimal to scaled integer conversion correctly validates the expected scale (18), extracts the big integer value, and properly applies the sign. This ensures precision alignment with EVM decimal standards.
extensions/tn_attestation/integration_test.go (1)
45-56: Canonical payload builder aligns with the new big-endian schemeSwitching the fixture to
BuildCanonicalPayloadwith deterministic provider/stream IDs matches the production path and keeps the integration test in lockstep with the contract-facing format. Looks solid.extensions/tn_attestation/canonical_test.go (1)
11-40: Happy path coverage stays comprehensiveThe test updates exercise
BuildCanonicalPayloadand verify the digest against the raw bytes, so we still cover all fields under the new encoding. All good here.extensions/tn_attestation/processor_test.go (1)
239-250: Hash material now mirrors canonical payload exactlyUpdating the hash fixture to use the same big-endian length-prefix helper removes the previous divergence between tests and production. Nice catch.
extensions/tn_utils/precompiles.go (1)
138-153: New ABI precompile integrates smoothlyAdding
canonical_to_datapoints_abiwith the existing helpers cleanly exposes the ABI encoder to SQL. Registration and handler wiring look spot on.
Time Submission Status
|
…n workflow - Updated the harness integration test to replace byte arrays with string representations for dataProvider and streamID, improving readability and consistency with expected formats. - Refactored the requestStreamAttestation function to return encoded action arguments, facilitating better validation in end-to-end tests. - Enhanced the testCompleteStreamAttestationWorkflow function to include detailed validation of canonical payload fields, ensuring accurate verification of attestation results. - Introduced new helper functions for decoding payload strings and handling argument types, improving code clarity and maintainability. These changes enhance the reliability and clarity of the attestation testing framework, ensuring robust validation of the attestation process.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/extensions/tn_attestation/attestation_e2e_test.go (1)
422-424: Remove ASCII fallback to avoid masking bad payloads.If the payload string isn’t valid hex/base64 we immediately return the raw ASCII bytes, so we’re back to slicing textual data—the bug from the earlier review. Let this surface as an error instead so the test fails when the payload isn’t real binary.
- // As a last resort, treat the original string as raw bytes. - return []byte(raw), nil + return nil, fmt.Errorf("unrecognized payload encoding: %q", raw)
🧹 Nitpick comments (1)
extensions/tn_attestation/harness_integration_test.go (1)
353-397: Consider the implications of requester-based lookup.The function changed from querying by
attestation_hash(which is unique) to querying byrequesterwithORDER BY created_height DESC, request_tx_id DESC LIMIT 1. While this works in the current test context where each requester has only one attestation, it's less precise and could be fragile if the test is extended to create multiple attestations for the same requester.Consider these options:
- Keep current approach if the test will always use unique requesters or only query the latest attestation.
- Revert to hash-based lookup for more precise row identification:
-func fetchAttestationRowHarness(t *testing.T, ctx context.Context, platform *kwilTesting.Platform, requester []byte) harnessAttestationRow { +func fetchAttestationRowHarness(t *testing.T, ctx context.Context, platform *kwilTesting.Platform, attestationHash []byte) harnessAttestationRow { // ... err := platform.Engine.Execute(engineCtx, platform.DB, ` -WHERE requester = $requester -ORDER BY created_height DESC, request_tx_id DESC -LIMIT 1; -`, map[string]any{"requester": requester}, func(row *common.Row) error { +WHERE attestation_hash = $attestation_hash; +`, map[string]any{"attestation_hash": attestationHash}, func(row *common.Row) error {Or document the assumption that each requester has only one attestation in this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
extensions/tn_attestation/harness_integration_test.go(7 hunks)tests/extensions/tn_attestation/attestation_e2e_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/extensions/tn_attestation/attestation_e2e_test.go (2)
extensions/tn_attestation/canonical.go (1)
ParseCanonicalPayload(63-105)extensions/tn_utils/serialization.go (2)
DecodeActionArgs(85-130)EncodeActionArgs(45-76)
extensions/tn_attestation/harness_integration_test.go (2)
extensions/tn_utils/serialization.go (2)
EncodeActionArgs(45-76)EncodeQueryResultCanonical(145-184)extensions/tn_attestation/canonical.go (2)
BuildCanonicalPayload(38-58)ParseCanonicalPayload(63-105)
⏰ 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)
extensions/tn_attestation/harness_integration_test.go (4)
96-98: LGTM - dataProvider and streamID standardization.The change from fixed byte slices to generated Ethereum address and stream ID aligns with the PR's standardization objectives. Using
util.GenerateStreamIdand string representations improves consistency with the production code path.
202-204: LGTM - Algorithm and field validations updated for standardization.The algorithm field validation changed from
1to0to correctly represent secp256k1 (as documented in line 128). TheDataProviderandStreamIDassertions are updated to match the new canonical payload structure using Ethereum addresses and string-based stream IDs. These changes align with the PR's standardization objectives.
216-217: LGTM - Improved error handling for key generation.Adding proper error checking for
kcrypto.GenerateSecp256k1Keyimproves test robustness. Previously, a key generation failure could have gone unnoticed, leading to confusing test failures downstream.
106-179: SQL migrationrequest_attestationis adequately tested elsewhere.Verification confirms that
request_attestationis thoroughly tested in:
tests/streams/attestation/attestation_request_test.go— direct unit test of the SQL actiontests/streams/attestation/test_helpers.go— helper functions used across test suitestests/extensions/tn_attestation/attestation_e2e_test.go— end-to-end client executionThe manual payload construction in this harness test appropriately isolates testing of
BuildCanonicalPayload, while the SQL migration path is exercised separately and comprehensively elsewhere.
|
I'll keep it as a non-draft to receive checks from coderabbit |
- Updated the harness integration test to manually construct the attestation using the BuildCanonicalPayload function, ensuring accurate verification of attestation bytes. - Refactored the request_attestation test to replace byte arrays with hex string representations for dataProvider, improving consistency and readability. - Enhanced validation in the runAttestationHappyPath function to ensure correct data types for return values, improving test reliability. - Modified the SetupTestAction function to return a more detailed result structure, enhancing clarity in the test setup. These changes improve the robustness and maintainability of the attestation testing framework, ensuring accurate validation of the attestation process.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
extensions/tn_attestation/harness_integration_test.go(7 hunks)tests/streams/attestation/attestation_request_test.go(4 hunks)tests/streams/attestation/test_helpers.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
extensions/tn_attestation/harness_integration_test.go (2)
extensions/tn_utils/serialization.go (2)
EncodeActionArgs(45-76)EncodeQueryResultCanonical(145-184)extensions/tn_attestation/canonical.go (2)
BuildCanonicalPayload(38-58)ParseCanonicalPayload(63-105)
tests/streams/attestation/attestation_request_test.go (4)
tests/streams/attestation/test_helpers.go (2)
TestDataProviderHex(28-28)TestStreamID(27-27)extensions/tn_utils/serialization.go (1)
EncodeQueryResultCanonical(145-184)extensions/tn_utils/datapoints.go (1)
EncodeDataPointsABI(32-64)extensions/tn_attestation/canonical.go (1)
BuildCanonicalPayload(38-58)
🔇 Additional comments (13)
extensions/tn_attestation/harness_integration_test.go (3)
94-96: Comment update addresses prior feedback.The updated comment accurately describes that the test manually constructs the attestation using BuildCanonicalPayload, rather than claiming to request through the live migration. This resolves the previously flagged discrepancy.
203-206: Canonical payload assertions updated correctly.The algorithm value of 0 corresponds to secp256k1 (as noted in the comment at line 129), and the DataProvider/StreamID assertions now correctly validate the new canonical payload structure with big-endian encoding.
354-399: Function now fetches by requester (latest attestation).The signature changed from hash-based lookup to requester-based lookup with
ORDER BY created_height DESC, request_tx_id DESC LIMIT 1. This returns the most recent attestation for the requester rather than a specific attestation by hash. For this test harness where only one attestation per requester exists, this is acceptable. The addition of thefoundflag to verify a row was returned is a good defensive check.If future tests create multiple attestations per requester and need to retrieve a specific one, consider adding an optional hash parameter or creating a separate hash-based fetch helper.
tests/streams/attestation/test_helpers.go (5)
8-9: LGTM! Imports support hex decoding.The new imports are used appropriately by the
mustHexToByteshelper function.
39-46: LGTM! Hex decoding helper is correct.The implementation correctly handles the
0xprefix and uses panic for test initialization failures, which is appropriate for fail-fast test setup.
158-159: LGTM! Standardized test constants eliminate magic strings.Using
TestDataProviderHexandTestStreamIDconstants improves test maintainability and ensures consistency across all attestation tests.Also applies to: 215-216
243-244: Verify NUMERIC(36,18) precision is intentional.The
NUMERIC(36,18)return type provides 36 total digits with 18 decimal places. While 18 decimals align with Ethereum's wei precision (supporting EVM compatibility per PR objectives), 36 total digits allows values up to ~10^18, which may be excessive. Ensure this precision matches your domain requirements and is documented if it's a deliberate design choice.
27-28: Review comment remains valid but requires developer judgment.The address collision is confirmed:
TestDataProviderHexand the owner inNewTestAddresses()both use "0x0000000000000000000000000000000000000b11". The collision appears intentional (not a typo), while other test addresses (Requester1,Requester2) are distinct.The concern about potential confusion is architectural rather than a bug: in real attestation systems, owner and data provider are typically different entities. Using identical addresses simplifies tests but may not adequately exercise scenarios where these roles are distinct. No test logic differentiates based on address equality, and no validations require them to be different—the code handles identical addresses without issue.
Developer verification needed: Consider whether using separate addresses (e.g., reusing an existing requester address) better represents production scenarios where owner and data provider are distinct entities.
tests/streams/attestation/attestation_request_test.go (5)
7-7: LGTM! New imports support standardized canonical payload construction.The imports enable the refactored code to use
BuildCanonicalPayloadand proper decimal/address handling.Also applies to: 13-13, 15-15, 19-19
46-47: LGTM! Using standardized test constants.Referencing
TestDataProviderHexandTestStreamIDfrom test helpers ensures consistency across the test suite.
64-70: LGTM! Enhanced type safety and defensive copying.The explicit length check, type assertions, and defensive copy of the hash bytes improve test robustness and provide clearer error messages.
81-93: Verify hardcoded values match action definition.The canonical result construction uses:
- Line 81: Decimal formatting with fixed 18 zeros (assumes integer values)
- Line 85: Hardcoded timestamp of
1These should match the test action's return values. From test_helpers.go line 244, the action returns
1asevent_time, which aligns correctly. The decimal format works for integerattestedValuebut would fail for fractional values.Consider adding a comment explaining the decimal format assumption if this pattern is reused elsewhere:
// Construct decimal with 18 decimal places (assumes integer input) valueDecimal := types.MustParseDecimal(fmt.Sprintf("%d.%018d", attestedValue, 0))
95-105: Excellent refactoring to use standardized BuildCanonicalPayload!This change replaces the local
buildExpectedCanonicalPayloadhelper with the centralizedattestation.BuildCanonicalPayload, which:
- Uses big-endian length prefixes (per PR objectives)
- Improves EVM compatibility
- Reduces code duplication
- Ensures consistency across the codebase
The parameters are correctly mapped to the standardized function signature.
- Introduced a new test, TestGoldenPayloadFixtureMatches, to validate the canonical payload, signature, and payload hex against a predefined golden fixture. - Added a new JSON file, attestation_golden.json, containing expected values for canonical payload, signature, and payload hex, along with associated arguments and results. - Enhanced the test to read the fixture data, encode action arguments, and verify the computed values against the expected results, improving the robustness of attestation tests. These changes enhance the accuracy and reliability of attestation validation by ensuring consistency with expected outputs.
- Updated the harness integration test to use a new variable, persistedHash, for comparing the stored attestation hash, ensuring accurate validation. - Enhanced the error message for the hash comparison to provide clearer context in case of failure. These changes improve the reliability of the attestation tests by ensuring that the correct values are being compared.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
extensions/tn_attestation/harness_integration_test.go (7)
104-106: Drop redundant temp var.Inline requestTxID assignment to avoid an unused/extra variable.
- expectedTxID := engineCtx.TxContext.TxID - requestTxID := expectedTxID + requestTxID := engineCtx.TxContext.TxID
123-136: Disambiguate variable names: “rows canonical” vs “payload canonical”.Rename locals so it’s obvious which bytes represent the row-encoding vs the full canonical payload, reducing mental overhead when reading later assertions and SQL params.
- resultCanonical, err := tn_utils.EncodeQueryResultCanonical(dispatchRows) + rowsCanonical, err := tn_utils.EncodeQueryResultCanonical(dispatchRows) require.NoError(t, err, "encode canonical query result") providerAddr := util.Unsafe_NewEthereumAddressFromString(dataProvider) - canonicalBytes := BuildCanonicalPayload( + canonicalPayload := BuildCanonicalPayload( 1, // version 0, // algorithm (secp256k1) uint64(engineCtx.TxContext.BlockContext.Height), providerAddr.Bytes(), []byte(streamID), uint16(testActionID), argsBytes, - resultCanonical, + rowsCanonical, )- "result_canonical": canonicalBytes, + "result_canonical": canonicalPayload,Also applies to: 173-178
126-136: Replace magic numbers with named constants for version/algorithm.Use local constants to avoid drift and self-document intent.
Add near the other test constants:
const ( canonicalVersion uint8 = 1 algorithmSecp256k1 uint8 = 0 )Then apply:
- canonicalPayload := BuildCanonicalPayload( - 1, // version - 0, // algorithm (secp256k1) + canonicalPayload := BuildCanonicalPayload( + canonicalVersion, + algorithmSecp256k1,
138-143: Assert pre-store digest equivalence.Sanity-check that the computed attestationHash equals the payload’s SigningDigest before persistence.
payloadStruct, err := ParseCanonicalPayload(canonicalBytes) require.NoError(t, err, "parse canonical payload for hashing") hashArray := computeAttestationHash(payloadStruct) attestationHash := append([]byte(nil), hashArray[:]...) + require.Equal(t, attestationHash, payloadStruct.SigningDigest(), "hash must match signing digest (pre-store)")
185-192: Unnecessary reassign of attestationHash.After asserting equality, reassigning attestationHash to persistedHash adds no value.
- require.Equal(t, attestationHash, persistedHash, "returned attestation hash should match stored hash") - attestationHash = persistedHash + require.Equal(t, attestationHash, persistedHash, "returned attestation hash should match stored hash")
201-206: Strengthen canonical verification: validate + block height.Add explicit ValidateForEVM and assert the block height to lock the wire format tighter.
payload, err := ParseCanonicalPayload(stored.resultCanonical) require.NoError(t, err, "canonical payload should be parseable") + require.NoError(t, payload.ValidateForEVM(), "canonical payload must be EVM-valid") require.Equal(t, uint8(1), payload.Version) require.Equal(t, uint8(0), payload.Algorithm) + require.Equal(t, uint64(engineCtx.TxContext.BlockContext.Height), payload.BlockHeight) require.Equal(t, providerAddr.Bytes(), payload.DataProvider) require.Equal(t, []byte(streamID), payload.StreamID)
355-377: Clarify helper intent with a doc comment (latest-by-requester).Add a brief comment so future readers don’t assume this fetches by hash.
+// fetchAttestationRowHarness returns the latest attestation row for a requester, +// ordered by (created_height DESC, request_tx_id DESC). Intended for single-row +// workflows in this harness; not a general “fetch by hash” lookup. func fetchAttestationRowHarness(t *testing.T, ctx context.Context, platform *kwilTesting.Platform, requester []byte) harnessAttestationRow {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
extensions/tn_attestation/harness_integration_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
extensions/tn_attestation/harness_integration_test.go (2)
extensions/tn_utils/serialization.go (2)
EncodeActionArgs(45-76)EncodeQueryResultCanonical(145-184)extensions/tn_attestation/canonical.go (2)
BuildCanonicalPayload(38-58)ParseCanonicalPayload(63-105)
⏰ 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: acceptance-test
🔇 Additional comments (1)
extensions/tn_attestation/harness_integration_test.go (1)
94-97: Comment fix LGTM.Updated note now accurately describes the manual canonical construction path used by the test.
2 participants
Description
Related Problem
How Has This Been Tested?
Summary by CodeRabbit
New Features
Tests
Documentation
Bug Fixes / Changes