test: strategy integration tests #68
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several updates across the codebase. A new entry is added to the default strategy mapping, associating a specific chain ID with an extra address and its hex value. The event factory now defines the block number parameter as optional, setting a default value when omitted. A new integration test suite for the orchestrator has been implemented to evaluate the processing of various strategy-related events using defined mocks and spies. In addition, multiple strategy handler modules and their corresponding tests are modified: the conversion of the block timestamp has been changed to use the value directly (in milliseconds) instead of multiplying by 1000. This adjustment affects the way that timestamps are interpreted both in production code and in tests, ensuring a consistent representation of time across the system. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/data-flow/test/integration/strategy.integration.spec.ts (2)
55-55: Enhance test descriptions for better clarity.Consider making test descriptions more descriptive by focusing on the expected behavior rather than the implementation detail of "processing". For example:
- "process Distributed event" → "updates round total when distribution occurs"
- "process DistributionUpdated event" → "updates round data when distribution is updated"
Also applies to: 112-112, 191-191, 259-259, 354-354, 436-436
59-63: Consider extracting test data constants.Consider extracting commonly used test values into named constants for better maintainability and readability. For example:
- Amount values
- Token addresses
- Metadata values
Also applies to: 115-118, 194-199, 263-267, 358-363, 439-444
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/data-flow/test/integration/helpers/dependencies.ts(1 hunks)packages/data-flow/test/integration/helpers/eventFactory.ts(2 hunks)packages/data-flow/test/integration/strategy.integration.spec.ts(1 hunks)packages/processors/src/processors/strategy/directGrantsLite/handlers/registered.handler.ts(1 hunks)packages/processors/src/processors/strategy/directGrantsSimple/handlers/registered.handler.ts(1 hunks)packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts(1 hunks)packages/processors/test/strategy/directGrantsLite/handlers/registered.handler.spec.ts(1 hunks)packages/processors/test/strategy/directGrantsSimple/handlers/registered.handler.spec.ts(1 hunks)packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/processors/src/processors/strategy/directGrantsSimple/handlers/registered.handler.tspackages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.tspackages/processors/test/strategy/directGrantsSimple/handlers/registered.handler.spec.tspackages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.spec.tspackages/processors/src/processors/strategy/directGrantsLite/handlers/registered.handler.tspackages/data-flow/test/integration/strategy.integration.spec.tspackages/processors/test/strategy/directGrantsLite/handlers/registered.handler.spec.tspackages/data-flow/test/integration/helpers/dependencies.tspackages/data-flow/test/integration/helpers/eventFactory.ts
`**/*.spec.ts`: Review the unit test files with the followin...
**/*.spec.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/processors/test/strategy/directGrantsSimple/handlers/registered.handler.spec.tspackages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.spec.tspackages/data-flow/test/integration/strategy.integration.spec.tspackages/processors/test/strategy/directGrantsLite/handlers/registered.handler.spec.ts
🔇 Additional comments (9)
packages/data-flow/test/integration/helpers/eventFactory.ts (1)
99-99: LGTM! Consistent parameter handling.The change to make
blockNumberoptional with a default value aligns with the pattern used in other event factory functions, improving API consistency.Also applies to: 109-109
packages/data-flow/test/integration/helpers/dependencies.ts (1)
116-119: LGTM! Strategy mapping entry added.The new strategy mapping entry follows the established pattern and is properly formatted.
packages/data-flow/test/integration/strategy.integration.spec.ts (1)
20-527: Well-structured and comprehensive test suite!The test suite is well-implemented with:
- Proper test setup and cleanup
- Comprehensive coverage of different event types
- Good use of mocks and spies
- Detailed assertions
- Proper error handling
packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts (1)
72-72:❓ Verification inconclusive
Verify the blockTimestamp format.
The change from
new Date(blockTimestamp * 1000)tonew Date(blockTimestamp)assumes thatblockTimestampis already in milliseconds. Please verify this assumption.
🏁 Script executed:
#!/bin/bash # Description: Check the format of blockTimestamp in the codebase # Look for any timestamp conversions or related comments rg -A 2 'blockTimestamp' --type tsLength of output: 183
Verify BlockTimestamp Assumption – Manual Confirmation Needed
The change from using
new Date(blockTimestamp * 1000)tonew Date(blockTimestamp)hinges on the assumption thatblockTimestampis provided in milliseconds rather than in seconds. However, our initial investigation (using ripgrep) did not return any useful results due to filtering issues.Next Steps:
Search for Conversion Logic: Check if there are any instances of
blockTimestamp * 1000elsewhere in the codebase that might indicate the previous expectation of a seconds-based timestamp.Examine the Specific File: In
packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.tsverify near line 72 (or elsewhere) how
blockTimestampis derived and whether any conversion (multiplication by1000) is performed before its usage.Re-run More Targeted Searches: For example, you can run the following shell script to help locate any conversions related to
blockTimestampand confirm the file’s content:#!/bin/bash # Check if the expected file exists find . -type f -path "packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts" || echo "registered.handler.ts file not found" # Search for usage of blockTimestamp multiplied by 1000 anywhere in the repository rg -n "blockTimestamp\s*\*\s*1000" || echo "No occurrences of blockTimestamp * 1000 found" # Display occurrences of blockTimestamp in the specified file grep -n "blockTimestamp" packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts || echo "No blockTimestamp references found in registered.handler.ts"Please run the above script and manually verify that:
- There is no residual expectation of seconds-based timestamps elsewhere.
- The value in
blockTimestampis indeed provided in milliseconds.Once confirmed, no further action on this part is necessary. Otherwise, adjust the conversion accordingly.
Manual Verification Needed – Confirm BlockTimestamp Format
The revised shell script above is intended to help verify whether the change from using
new Date(blockTimestamp * 1000)tonew Date(blockTimestamp)is valid—that is, whetherblockTimestampis indeed already in milliseconds. Please run this script and review its output for:
- Any occurrences of
blockTimestampindicating its origin or manipulation.- Any patterns multiplying
blockTimestampby 1000 which would imply it is initially in seconds.Based on the results, update the conversion logic if necessary.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for any usage of "blockTimestamp" across the repository without file type filtering echo "Searching for blockTimestamp occurrences (no file type filter):" rg -A 2 "blockTimestamp" || echo "No occurrences found." # Search the repository for any instance where blockTimestamp is multiplied by 1000 echo "Searching for blockTimestamp multiplication pattern:" rg -n "blockTimestamp\s*\*\s*1000" || echo "No multiplication pattern found." # Optionally, check the content of the specific file for context on blockTimestamp usage echo "Checking registered.handler.ts for blockTimestamp references:" grep -n "blockTimestamp" packages/processors/src/processors/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts || echo "No blockTimestamp references found in registered.handler.ts."Length of output: 1129
BlockTimestamp Format Verification – Manual Confirmation Required
Our automated search did not return any instances of multiplying
blockTimestampby 1000, and we couldn’t locate the file at the specified path. This suggests that either the file path is different than expected or that the conversion was already updated elsewhere. Please verify the following manually:
- Confirm that the file under review (or its correct location) indeed contains the change from
new Date(blockTimestamp * 1000)tonew Date(blockTimestamp).- Verify that the value of
blockTimestampis already provided in milliseconds throughout the codebase.- Ensure there are no other parts of the repository still expecting a seconds-based timestamp (e.g., by checking for any conversion logic like
blockTimestamp * 1000).Based on your manual verification, adjust the conversion if necessary.
packages/processors/src/processors/strategy/directGrantsSimple/handlers/registered.handler.ts (1)
78-78:❓ Verification inconclusive
Ensure consistent timestamp handling.
The timestamp handling has been changed consistently across all handlers. However, please verify that this change aligns with how timestamps are handled in other parts of the system.
🏁 Script executed:
#!/bin/bash # Description: Check for other timestamp handling patterns # Look for Date constructor usage patterns ast-grep --pattern 'new Date($timestamp)'Length of output: 43
Below is an updated shell script to widen the search for timestamp handling patterns across the repository. The previous ast-grep command didn’t produce any output, so please run this alternative using
rgto ensure we capture all instances of thenew Date(usage:Once you run this command, please review the output to verify that the usage in
packages/processors/src/processors/strategy/directGrantsSimple/handlers/registered.handler.tsat line 78
(updatedAt: new Date(blockTimestamp)) aligns with how timestamps are processed elsewhere in the system.
🏁 Script executed:
[run_scripts] #!/bin/bash # Searching for consistent timestamp handling patterns across the repository using rg. rg "new Date(" -n .Length of output: 212
Re-run the Timestamp Consistency Check
The earlier command encountered a regex error due to unescaped parenthesis. Please run the updated script below to search for all instances of the Date constructor across the repository. This will help you verify that the timestamp handling in
packages/processors/src/processors/strategy/directGrantsSimple/handlers/registered.handler.ts(line 78) is consistent with other parts of the system.
🏁 Script executed:
[run_scripts] #!/bin/bash # Searching for consistent use of new Date(...) with proper escaping rg "new Date\\(" -n .Length of output: 144
Timestamp Handling Consistency – Further Manual Verification Needed
The automated searches did not yield additional instances of
new Date(usage that conflict with the change inupdatedAt: new Date(blockTimestamp). However, since the automated output was inconclusive, please manually verify that the timestamp handling in this handler aligns with the approach taken throughout the system.packages/processors/src/processors/strategy/directGrantsLite/handlers/registered.handler.ts (1)
77-77: LGTM! Consistent timestamp handling.The timestamp handling is consistent with other handlers in the system.
packages/processors/test/strategy/directGrantsSimple/handlers/registered.handler.spec.ts (1)
87-87: LGTM! Test updated to match implementation.The test has been correctly updated to reflect the new timestamp handling approach.
packages/processors/test/strategy/directGrantsLite/handlers/registered.handler.spec.ts (1)
91-91:❓ Verification inconclusive
LGTM! Correct timestamp handling.
The removal of
* 1000multiplication is the right approach since blockchain timestamps are typically in milliseconds.Let's confirm the timestamp unit in the codebase:
🏁 Script executed:
#!/bin/bash # Search for blockTimestamp usage patterns to verify the timestamp unit rg -A 2 'blockTimestamp' --type tsLength of output: 183
🏁 Script executed:
#!/bin/bash # Re-run search for blockTimestamp in the specific test file to ensure we gather the proper context. rg -A 2 'blockTimestamp' packages/processors/test/strategy/directGrantsLite/handlers/registered.handler.spec.tsLength of output: 238
Timestamp Handling Verification (Manual Check Requested)
The updated code using
new Date(mockEvent.blockTimestamp)appears correct based on the assumption that blockchain timestamps are provided in milliseconds. However, our automated search for occurrences ofblockTimestampdid not return any results—likely due to file relocation or naming differences—which makes it difficult to conclusively confirm the unit across the codebase.
- Action Items:
- Manually verify that the event’s
blockTimestampis indeed in milliseconds and no further scaling (like multiplying by 1000) is needed.- Confirm that the referenced test file location is correct or update the file path if it has moved.
Overall, the removal of the
* 1000multiplication is intended since timestamps are expected in milliseconds. Please ensure that this assumption holds true in all relevant parts of the application.packages/processors/test/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.spec.ts (1)
111-111: LGTM! Consistent timestamp handling.The timestamp handling is now consistent with other handlers and correctly preserves the millisecond precision.
| params: { | ||
| recipientId: mockAnchorAddress, | ||
| amount: "1000000000000000000", // 1 ETH | ||
| token: "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE", // ETH |
There was a problem hiding this comment.
idk this doesn't look like ETH to me haha
There was a problem hiding this comment.
jajaj it's actually the way we represent the native token address (since it doesnt have an address) but i can delete the comment to avoid confussion
| eventName: "DistributedWithRecipientAddress", | ||
| params: { | ||
| recipientId: "0xrecipient", | ||
| recipientAddress: "0xrecipient", |
There was a problem hiding this comment.
I think generally we should use a valid mock address where we have an address field because I've gotten issues before in EBO where we were using invalid addresses and it caused test errors later on when we were adding address validation, wdyt?
# 🤖 Linear Closes GIT-265 GIT-262 ## Description Write integration tests for Strategy events on Orchestrator level (continuation of #67). Also, fix some issues on Registration handler regarding Date and BlockTimestamp conversion <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Expanded strategy mapping to support additional entries. - **Refactor** - Enhanced event handling by making a key parameter optional with a default value. - Refined timestamp processing to ensure consistent and accurate date representations. - **Tests** - Introduced new integration tests to validate and monitor strategy event processing and time accuracy. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 Linear
Closes GIT-265 GIT-262
Description
Write integration tests for Strategy events on Orchestrator level (continuation of #67). Also, fix some issues on Registration handler regarding Date and BlockTimestamp conversion
Summary by CodeRabbit