chore: add transaction ledger schema and helper actions#1254
Conversation
Introduce a new SQL migration for transaction ledger management, including the creation of tables for transaction methods, events, and distributions. Implement helper actions for recording transaction events, appending fee distributions, retrieving transaction details, and listing transaction fees. This enhances the system's capability to manage transaction fees and their distribution effectively.
WalkthroughAdds a transaction-events ledger (schema + actions), threads fee collection and leader-fee transfers into multiple write and bridge actions, records transaction events after those operations, and adds end-to-end tests and test helpers to validate ledger and query surfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Action as Domain Action
participant FeeLogic as Fee & Leader Logic
participant Bridge as ERC20 Bridge
participant TxEvent as record_transaction_event
participant Ledger as Transaction Ledger
Caller->>Action: invoke (create_streams / insert_records / set_taxonomy / transfer / request_attestation)
Action->>FeeLogic: check exemption & compute total_fee
alt caller not exempt
FeeLogic->>FeeLogic: ensure leader_sender present (else error)
FeeLogic->>FeeLogic: derive leader_hex
FeeLogic->>Bridge: transfer total_fee to leader_hex
Bridge-->>FeeLogic: transfer confirmed
end
Action->>Action: perform core operation
Action->>TxEvent: record_transaction_event(method_id, fee_total, fee_recipient, metadata)
TxEvent->>Ledger: insert/update transaction_events
TxEvent->>Ledger: insert transaction_event_distributions (if recipient & amount > 0)
Ledger-->>TxEvent: ack
TxEvent-->>Action: done
Action-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Time Submission Status
|
This update introduces several improvements to transaction event management across various actions. Key changes include the addition of fee calculations and recipient tracking for stream creation, record insertion, and taxonomy insertion actions. The `record_transaction_event` function is now utilized to log transaction details, including fees and recipients, enhancing the system's ability to manage and audit transaction fees effectively. Additionally, new tests have been added to ensure the integrity of these features.
…ndling This commit refines the `get_last_transactions_v2` action to improve the retrieval of transaction details, including fee distributions. The implementation now aggregates fee distribution data from the `transaction_event_distributions` table, ensuring accurate reporting of fees associated with each transaction. Additionally, the `transaction_events_ledger_test.go` file has been updated to reflect these changes, enhancing test coverage for the new distribution logic and ensuring the integrity of transaction data retrieval.
…ee handling This commit simplifies the `record_transaction_event` and `append_fee_distribution` actions by removing unnecessary variables and redundant logic related to fee summaries. The `list_transaction_fees` and `get_last_transactions_v2` actions have been updated to enhance clarity and efficiency in retrieving transaction details, including fee distributions. Additionally, the test suite has been cleaned up to remove outdated debug logging, ensuring a more focused and efficient testing process.
This commit updates the `list_transaction_fees` action to improve the handling of transaction event distributions by introducing a subquery for filtering recipients based on the transaction mode. Additionally, the test suite for transaction events has been expanded to include checks for bonus fee distributions and ensure the integrity of transaction data retrieval. These changes enhance clarity and accuracy in fee reporting across various transaction scenarios.
…ion retrieval logic This commit modifies the `get_last_transactions_v2` action to replace the `$data_provider` parameter with `$wallet`, improving clarity and functionality. The implementation now includes additional checks for valid Ethereum addresses and limits on the number of transactions retrieved. Furthermore, the SQL logic has been refined to aggregate transaction event distributions, ensuring accurate reporting. Corresponding updates have been made to the test suite to validate these changes and enhance coverage for transaction retrieval scenarios.
![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: 2
🧹 Nitpick comments (2)
internal/migrations/024-attestation-actions.sql (1)
157-162: Consider using a named constant for method_id.The hardcoded
6for method_id reduces maintainability. If transaction method IDs are defined in the schema (e.g., in026-transaction-schemas.sql), consider referencing them symbolically or adding a comment explaining the mapping.internal/migrations/004-composed-taxonomy.sql (1)
126-131: Consider using a named constant for method_id.The hardcoded
3for method_id reduces maintainability. Consider adding a comment documenting that3maps tosetTaxonomies, or use a symbolic reference if method IDs are defined in the schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/migrations/001-common-actions.sql(4 hunks)internal/migrations/003-primitive-insertion.sql(3 hunks)internal/migrations/004-composed-taxonomy.sql(3 hunks)internal/migrations/010-get-latest-write-activity.sql(1 hunks)internal/migrations/024-attestation-actions.sql(1 hunks)internal/migrations/026-transaction-schemas.sql(1 hunks)internal/migrations/027-transaction-actions.sql(1 hunks)internal/migrations/901-utilities.sql(0 hunks)internal/migrations/erc20-bridge/001-actions.sql(2 hunks)internal/migrations/erc20-bridge/002-public-transfer-actions.sql(1 hunks)tests/streams/transaction_events_ledger_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/migrations/901-utilities.sql
🧰 Additional context used
🧠 Learnings (5)
📚 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
📚 Learning: 2025-10-23T07:00:26.796Z
Learnt from: williamrusdyputra
Repo: trufnetwork/node PR: 1228
File: internal/migrations/erc20-bridge/000-extension.sql:16-19
Timestamp: 2025-10-23T07:00:26.796Z
Learning: In internal/migrations/erc20-bridge/000-extension.sql, the active sepolia_bridge configuration is intentionally for test environments only and does not include the distribution_period parameter. The commented sepolia_bridge and ethereum_bridge blocks with distribution_period are production configurations.
Applied to files:
internal/migrations/erc20-bridge/002-public-transfer-actions.sqlinternal/migrations/026-transaction-schemas.sqlinternal/migrations/erc20-bridge/001-actions.sql
📚 Learning: 2025-09-12T01:19:04.545Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.545Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
tests/streams/transaction_events_ledger_test.gointernal/migrations/erc20-bridge/001-actions.sql
📚 Learning: 2025-08-20T12:22:14.250Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1113
File: internal/migrations/020-digest-init.sql:3-11
Timestamp: 2025-08-20T12:22:14.250Z
Learning: In the trufnetwork/node repository, for the digest_config table in internal/migrations/020-digest-init.sql, the maintainer prefers to leave the table without initial seed rows, allowing the extension to handle the empty state case at runtime rather than seeding default configuration values in the migration.
Applied to files:
internal/migrations/026-transaction-schemas.sql
🧬 Code graph analysis (1)
tests/streams/transaction_events_ledger_test.go (7)
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)tests/streams/utils/setup/roles.go (1)
AddMemberToRoleBypass(73-98)tests/streams/utils/setup/common.go (2)
CreateDataProvider(193-234)CreateDataProviderWithoutRole(243-295)extensions/tn_utils/serialization.go (1)
EncodeActionArgs(45-76)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-85)
⏰ 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 (18)
internal/migrations/004-composed-taxonomy.sql (2)
18-20: LGTM: Clean variable initialization.The explicit initialization of fee tracking variables follows best practices and maintains consistency with the broader fee collection pattern.
62-66: Good defensive validation of leader presence.Validating
@leader_senderearly and failing with a clear error message prevents downstream issues during fee transfer.internal/migrations/001-common-actions.sql (2)
62-97: Well-structured fee collection flow.The fee collection logic is cleanly separated and follows a consistent pattern: initialize tracking variables, validate leader presence, perform transfer, then record amounts for later event emission. This aligns well with the broader transaction ledger implementation.
395-400: Correct zero-fee event recording.Recording a transaction event with zero fee for
insert_metadataaligns with the PR objective of tracking all write operations, even those that don't charge fees. This ensures complete transaction history.internal/migrations/003-primitive-insertion.sql (1)
33-69: Consistent fee collection and event recording implementation.The implementation follows the established pattern across migration files: initialize variables, validate leader, collect fees, then record event. This consistency aids maintainability.
Also applies to: 115-120
internal/migrations/erc20-bridge/002-public-transfer-actions.sql (1)
5-42: Solid input validation for transfer operations.The implementation includes proper validation of Ethereum address format and positive amounts before processing transfers. The defensive check for leader availability prevents runtime errors during fee collection.
internal/migrations/010-get-latest-write-activity.sql (3)
12-84: Legacy implementation provides backward compatibility.The v1 implementation maintains compatibility by querying existing tables (streams, primitive_events, taxonomies, metadata) and aggregating results. The limit validation (capped at 100, default 6) provides reasonable safeguards against resource exhaustion.
162-203: Well-designed migration path.The wrapper maintains API compatibility while sourcing data from v1, with NULL values for new ledger fields. This allows gradual client migration to v2 without breaking existing consumers.
119-142: Index already exists; no action required.An index
tx_event_dist_rec_idxis defined ontransaction_event_distributions (recipient, tx_id)in migration 026, which covers the EXISTS subquery filter at lines 134–139. The index structure supports efficient lookups with constant recipient filtering and varying tx_id matching from the outer query. No table scan risk exists.tests/streams/transaction_events_ledger_test.go (4)
45-70: Robust test initialization with proper error handling.The test setup includes defensive error handling for idempotent operations (e.g., bridge instance already exists) and ensures clean initialization of roles, data providers, and the ERC20 bridge extension.
117-150: Direct distribution manipulation tests ledger flexibility.The test directly modifies
transaction_event_distributionsto verify that fee redistribution is correctly reflected in ledger queries. While this validates the flexibility of the ledger system, ensure that production code paths for fee distribution are also tested through normal action execution.
209-265: Clear and comprehensive expected outcomes.The expected results map provides well-structured assertions for all transaction types, making it easy to understand what the test validates and simplifying maintenance as new transaction types are added.
284-391: Thorough validation of ledger query APIs.The test validates multiple query paths (list_transaction_fees, get_last_transactions_v2, get_last_transactions_v1, get_transaction_event) with various filtering modes, ensuring comprehensive coverage of the ledger API surface.
internal/migrations/027-transaction-actions.sql (4)
87-134:append_fee_distributioncorrectly supports multi-recipient distributions.The action validates inputs, computes the next sequence via MAX(sequence) + 1, and inserts a new distribution row. This allows callers to build complex fee splits across multiple recipients.
Assuming Kwil's single-threaded transaction model, the sequence computation is safe. Confirm this is the intended design (no concurrent transaction execution within a single block).
136-216:get_transaction_eventaggregation approach is pragmatic but document the format.The action correctly:
- Validates and normalizes tx_id (adds 0x prefix if missing)
- Joins transaction_events with transaction_methods to resolve human-readable method names
- Aggregates distribution entries into a comma-separated
recipient:amountstringThe string concatenation is safe here (addresses and amounts have no special characters) and works well for simple queries. If richer distribution data becomes needed in the future (e.g., notes, per-recipient metadata), consider returning a proper result set instead.
219-329: Mode filtering and index coverage verified; code is sound for production use.The mode filter logic correctly branches by mode:
- 'paid': Filters by caller (fees initiated by wallet)
- 'received': Filters by fee_recipient or wallet in distribution recipients
- 'both': Union of both conditions
Existing indices provide strong coverage:
transaction_events_caller_idx(caller, block_height, tx_id) supports the 'paid' mode filter and ORDER BYtx_event_dist_rec_idx(recipient, tx_id) optimizes the EXISTS subqueries for distribution lookups- No full table scans expected on large datasets
The LEFT JOIN filter (lines 322–323) remains correct. Pagination enforcement is proper (limit 1–1000, offset ≥ 0). No additional performance optimization required.
1-85: Original review comment references incorrect action names that don't exist in the codebase.The verification reveals the original review comment names don't match the actual SQL action definitions:
- "deployStream" → actual action is
create_stream- "setTaxonomies" → actual action is
insert_taxonomy- "transferTN" → actual action is
transfer_stream_ownership- "withdrawTN" → no matching action found
- "setMetadata" → actual action is
insert_metadata- "insertRecords" ✓ (matches)
- "requestAttestation" ✓ (matches)
The review comment's verification request is based on non-existent method names. Before validating instrumentation claims, the actual write operations in the codebase must be identified and listed correctly.
Likely an incorrect or invalid review comment.
internal/migrations/026-transaction-schemas.sql (1)
10-53: Identifier lengths are within Kwil's limits; original concern is incorrect.The schema design is sound with proper referential integrity and CASCADE delete semantics. The identifier length concern in the original review is unfounded. Kwil uses PostgreSQL under the hood, which allows identifiers up to 63 bytes (NAMEDATALEN−1). The longest identifier here is
transaction_event_distributionsat 31 characters, well within the limit.Likely an incorrect or invalid review comment.
This commit modifies the `sepolia_bridge_tokens` and `ethereum_bridge_tokens` actions to utilize a standardized `$bridge_recipient` variable instead of directly referencing the `$recipient` parameter. This change enhances code clarity and consistency in the bridge withdrawal process. No functional changes were introduced, ensuring the actions continue to operate as intended.
…or multi-recipient fee recording This commit removes the `append_fee_distribution` action, streamlining the transaction event handling. A placeholder for a new action, `record_tx_event_split`, is introduced to facilitate future implementation of multi-recipient fee recording. This change aims to enhance clarity and prepare for more efficient fee distribution management in upcoming updates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/migrations/027-transaction-actions.sql (3)
87-97: Clarify or remove commented‑out future feature.The commented block (lines 87–97) suggests a planned
record_tx_event_splitaction for multi-recipient fee recording. This TODO should either be tracked in a separate issue, a JIRA ticket, or removed to reduce noise in the migration file. If it remains, add a comment linking to the tracking issue.
37-45: Simplify idempotency check using EXISTS.The idempotency safeguard currently iterates via FOR loop with a boolean flag. This is correct but less efficient than using EXISTS directly in an IF condition or a COUNT query.
Consider refactoring to:
$event_exists BOOL := FALSE; SELECT EXISTS(SELECT 1 FROM transaction_events WHERE tx_id = $tx_id) INTO $event_exists; IF NOT $event_exists { -- insert logic }Alternatively, if the dialect supports it, combine the check and insert into a single conditional insert pattern.
155-168: Consider using array aggregation for distribution string building.The distribution string is built via concatenation in a loop (lines 155–168). This works correctly but is verbose. If the database dialect supports array aggregation or string_agg functions, this could be more concise:
$distribution TEXT := COALESCE( STRING_AGG(recipient || ':' || amount::TEXT, ',' ORDER BY sequence), '' );(Adjust function name and syntax to match your dialect.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/027-transaction-actions.sql(1 hunks)
⏰ 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 (3)
internal/migrations/027-transaction-actions.sql (3)
277-287: Verify LEFT JOIN filter logic in list_transaction_fees.Line 285 applies the condition
NOT $mode_is_received OR recipient = $wallet_lowerto the LEFT JOINed distributions. When mode is 'paid' or 'both',$mode_is_receivedis FALSE, soNOT $mode_is_receivedis TRUE, which means ALL distribution rows are included without filtering by wallet. This may unintentionally expose fee distribution details for addresses other than the querying wallet.Confirm this is the intended scope. If distributions should always be scoped to the querying wallet or related recipients, adjust the condition to:
WHERE recipient = $wallet_lower
23-24: Function validation confirms implementation is sound; no action required.The
check_ethereum_address()function is properly defined ininternal/migrations/001-common-actions.sql(lines 548–571) and executes before the dependent migration 027. It validates that input is exactly 42 characters, starts with0x, and contains only hexadecimal characters (0–9, a–f, A–F). The function returnsBOOLconsistently and handles specified edge cases: empty strings fail the length check, mixed case is accepted, and the0xprefix is verified. While EIP-55 checksum validation is not implemented, this appears intentional and acceptable for the use case where case-insensitive validation suffices.
1-85: Verified: Context variables are automatically framework-provided; no explicit caller setup needed.The concern is unfounded.
@txid,@caller, and@heightare framework-provided context variables in the Kwil database layer, automatically populated for all PUBLIC actions executed as transactions. All nine invocations ofrecord_transaction_eventare within PUBLIC action definitions (entry points likeinsert_records,request_attestation,transfer_tn, etc.), guaranteeing these variables are available at execution time. No explicit caller initialization is required.The action's defensive validation (lines 12–14, 22–24) is appropriate and confirms the framework contract. The codebase shows no explicit
SET @txid,SET @caller, orSET @heightstatements, reinforcing that these are automatically provided—not manually managed by application code.Likely an incorrect or invalid review comment.
…e context creation This commit introduces a new function, `newTxContextWithLeader`, to standardize the creation of transaction contexts with a designated leader public key. The `EngineContext` initialization has been refactored across multiple files to utilize this new function, improving code clarity and reducing redundancy. Additionally, minor adjustments were made to test cases to ensure alignment with the updated context management, enhancing overall test coverage and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/streams/primitive_batch_insert_alignment_test.go(3 hunks)tests/streams/utils/procedure/execute.go(4 hunks)tests/streams/utils/setup/common.go(9 hunks)tests/streams/utils/setup/primitive.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T13:36:53.482Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1126
File: internal/benchmark/setup.go:217-219
Timestamp: 2025-08-29T13:36:53.482Z
Learning: In internal/benchmark/setup.go, the zero-value filtering (data.Value == 0.0) in batchInsertUsingMultiBatchWithGroups is intentional for reducing payload size during benchmarks and should be kept as-is per outerlook's decision.
Applied to files:
tests/streams/primitive_batch_insert_alignment_test.go
📚 Learning: 2025-08-20T12:18:52.236Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1113
File: extensions/tn_digest/internal/engine_ops.go:38-46
Timestamp: 2025-08-20T12:18:52.236Z
Learning: In the Kwil database system, TEXT columns are returned as string type, not []byte, so string type assertions are appropriate for TEXT column values.
Applied to files:
tests/streams/utils/setup/primitive.go
🧬 Code graph analysis (1)
tests/streams/primitive_batch_insert_alignment_test.go (1)
tests/streams/utils/setup/common.go (1)
NewEngineContext(77-79)
⏰ 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 (7)
tests/streams/primitive_batch_insert_alignment_test.go (1)
79-79: LGTM: Clean refactor to centralized context construction.The migration to
setup.NewEngineContext()standardizes how engine contexts are created for tests, ensuring consistent leader-based contexts across the test suite.tests/streams/utils/setup/primitive.go (1)
158-159: LGTM: Consistent migration to centralized context construction.All functions (
InsertMarkdownPrimitiveData,insertPrimitiveData,InsertPrimitiveDataMultiBatch,InsertTruflationDataBatch) have been consistently migrated to usenewEthEngineContext()followed by explicitTxIDassignment. This standardizes context creation while preserving transaction identity.Also applies to: 208-209, 307-308, 376-377
tests/streams/utils/setup/common.go (5)
39-52: Type assertion defensiveness: Consider the panic in defaultLeaderPublicKey.The type assertion to
*crypto.Secp256k1PublicKey(lines 46-49) includes a panic if the assertion fails. Whilecrypto.GenerateSecp256k1Key()should always return aSecp256k1PublicKey, the panic is reasonable for test code since this represents a fundamental setup failure that should be caught immediately rather than causing subtle downstream issues.
54-69: LGTM: Height normalization ensures valid block contexts.The height normalization (lines 55-57) ensures that block height is always ≥ 1, which aligns with blockchain semantics where genesis is block 1. This prevents invalid block heights in test contexts and ensures deterministic behavior.
76-79: Well-designed API: NewEngineContext provides clean abstraction.The public
NewEngineContext()function provides a clean, documented interface for creating engine contexts with deterministic leaders. This promotes consistent usage across the test suite.
117-117: LGTM: Comprehensive and consistent migration.All functions in this file (
UntypedCreateStream,CreateStreamsWithOptions,DeleteStream,CreateDataProvider,CreateDataProviderWithoutRole,GetStreamId) have been consistently migrated to usenewEthEngineContext(), ensuring uniform leader-based context construction across setup utilities.Also applies to: 168-168, 198-198, 226-226, 267-267, 346-346
307-314: Appropriate use of newTxContextWithLeader for bypass scenario.The
removeMemberFromRoleBypassfunction correctly usesnewTxContextWithLeaderdirectly (rather thannewEthEngineContext) because it needs to setOverrideAuthz: trueon theEngineContext. This demonstrates proper understanding of when to use the lower-level helper.
…context management This commit introduces a new `testctx` package to centralize the creation of transaction and engine contexts, improving code organization and reducing redundancy. The `newTxContextWithLeader` function has been replaced with `NewTxContextWithAuth` in the `testctx` package, streamlining the context setup across multiple files. Additionally, the refactoring enhances clarity and maintainability of the codebase, ensuring consistent context management throughout the testing utilities.
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)
tests/streams/utils/procedure/execute.go(19 hunks)tests/streams/utils/setup/common.go(9 hunks)tests/streams/utils/testctx/context.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/streams/utils/procedure/execute.go
🧰 Additional context used
🧬 Code graph analysis (2)
tests/streams/utils/testctx/context.go (1)
tests/streams/utils/setup/common.go (1)
NewEngineContext(36-38)
tests/streams/utils/setup/common.go (1)
tests/streams/utils/testctx/context.go (2)
NewEngineContext(54-57)NewTxContextWithAuth(36-51)
⏰ 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
This commit updates the height validation in the `NewTxContextWithAuth` function to ensure that negative heights are handled correctly. The condition has been changed from `height <= 0` to `height < 0`, setting a default height of 1 when the provided height is negative. This adjustment improves the robustness of transaction context creation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/streams/utils/testctx/context.go (1)
37-39: Consider normalizing height 0 to 1 as well.The current check only normalizes negative heights, but
height=0would be passed through unchanged. In blockchain contexts, height 0 typically represents genesis or an invalid state, and most operations expect height ≥ 1.Apply this diff to handle height 0:
- if height < 0 { + if height <= 0 { height = 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/streams/utils/testctx/context.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/utils/testctx/context.go (1)
tests/streams/utils/setup/common.go (1)
NewEngineContext(36-38)
⏰ 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 (5)
tests/streams/utils/testctx/context.go (5)
1-13: LGTM!Package declaration and imports are clean and all dependencies are used.
15-18: LGTM!Appropriate use of
sync.Oncefor thread-safe lazy initialization of the default leader key.
40-51: LGTM!The TxContext is properly constructed with all required fields populated from the parameters and platform.
53-57: LGTM!The function appropriately delegates to
NewTxContextWithAuthwith Ethereum-specific parameters correctly extracted from the address type.
20-33: No issues found—the non-deterministic key generation is acceptable.The
defaultLeaderPublicKey()function usessync.Onceto cache the randomly generated key once per test execution. This ensures the same key instance is reused throughout the entire test run, making it deterministic within each test execution even though the key itself is random. Tests across the codebase use this key for identity and proposer matching viabytes.Equal()comparisons, not by comparing against hardcoded values. This pattern is intentional and prevents test flakiness.
2 participants
Description
Related Problem
How Has This Been Tested?
Summary by CodeRabbit
New Features
Enhancements
Tests