Fix timestamp parsing in block header #156
Merged
Conversation
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
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Block as AlgorandBlock (source)
participant Util as algorandBlockToHeader()
participant Header as Header object
rect #e6f7ff
Block->>Util: pass block (timestamp in seconds)
note right of Util: compute timestamp_ms = timestamp * 1000\ncreate Date(timestamp_ms)
Util->>Header: return header { timestamp: Date(...), ... }
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)packages/node/src/algorand/utils.algorand.spec.ts (1)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (2)
packages/node/src/algorand/utils.algorand.ts (1)
16-23: LGTM! Timestamp conversion fix is correct.The multiplication by 1000 correctly converts Algorand's Unix timestamp (seconds) to milliseconds for the JavaScript Date constructor. However, consider making the comment more descriptive for maintainability.
Apply this diff to improve the comment:
- timestamp: new Date(block.timestamp * 1000), // Add MS + timestamp: new Date(block.timestamp * 1000), // Convert from seconds to millisecondspackages/node/src/algorand/utils.algorand.spec.ts (1)
98-102: LGTM! Test correctly validates the timestamp conversion fix.The test validates that the timestamp conversion from seconds to milliseconds produces the expected Date value. The test implementation is correct and will catch any regression in the timestamp conversion logic.
Consider moving this test to a separate describe block for "Block Header Conversion" since it's testing the
algorandBlockToHeaderfunction rather than transaction filtering. This would improve test organization:describe('Block Header Conversion', () => { let app: INestApplication; let api: AlgorandApiService; let block: AlgorandBlock; beforeAll(async () => { [app, api] = await prepareApiService(); block = await api.api.getBlockByHeight(30478896); }); afterAll(async () => { await app?.close(); }); it('can correctly get a block timestamp date', () => { const header = algorandBlockToHeader(block); expect(header.timestamp).toEqual(new Date('2023-07-13T10:44:35.000Z')); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node/src/algorand/utils.algorand.spec.ts(2 hunks)packages/node/src/algorand/utils.algorand.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/node/src/algorand/utils.algorand.spec.ts (1)
packages/node/src/algorand/utils.algorand.ts (1)
algorandBlockToHeader(16-23)
🔇 Additional comments (2)
packages/node/src/algorand/utils.algorand.spec.ts (1)
8-8: LGTM!The import addition is necessary for the new test case and correctly formatted.
packages/node/src/algorand/utils.algorand.ts (1)
85-98: I need to check the actual codebase to verify the timestamp format handling. Let me search for the AlgorandBlock type and how timestamps are processed.Based on my verification, I've found evidence supporting the reviewer's concern:
Algorand's block timestamp is in seconds since epoch, while SubQuery's timestamp field is stored as a unix timestamp in milliseconds.
The review comment correctly identifies a timestamp format inconsistency in the code:
- Line 21 (
algorandBlockToHeader) converts the timestamp to milliseconds- Line 92 (
filterBlockTimestamp) receivesblock.timestampdirectly in secondsThis appears to be a legitimate issue that needs verification against the actual
filterBlockTimestampimplementation to confirm the expected format.Confirm that
filterBlockTimestampexpects milliseconds format and fix line 92 accordingly.If
filterBlockTimestampfrom@subql/node-coreexpects milliseconds (consistent with SubQuery's timestamp storage), then line 92 should convert the timestamp to milliseconds before passing it to the function, similar to whatalgorandBlockToHeaderdoes at line 21.
stwiname
force-pushed
the
fix-block-timestamp
branch
from
938fa36 to
ed345cb
Compare
Contributor
Coverage reportCaution Test run failed
Test suite run failedFailed tests: 3/19. Failed suites: 2/4.Report generated by 🧪jest coverage report action from ed345cb |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Algorand block timestamps do not include MS so when converting to a date it would produce an incorrect time. This has now been fiexed by adding MS.
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests