feat: add created_timestamp to cctx status#2644
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant CCTX as CrossChainTx
participant Status as Status Struct
participant Tracker as Tracker Component
CCTX->>Status: Initialize new transaction
Status->>Status: Set CreatedTimestamp
Status->>Tracker: Log transaction creation
Tracker->>CCTX: Monitor transaction status
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2644 +/- ##
========================================
Coverage 70.35% 70.35%
========================================
Files 338 338
Lines 18599 18599
========================================
Hits 13085 13085
Misses 4922 4922
Partials 592 592
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
92-93: Clarify the purpose and usage ofcreated_timestamp.The comment indicates that
created_timestampis only populated on new transactions. Ensure that this is clearly documented in the protobuf file to avoid confusion.- // when the CCTX was created. only populated on new transactions. + // Timestamp indicating when the CCTX was created. This field is only populated on new transactions.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
x/crosschain/types/cross_chain_tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (4)
- proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1 hunks)
- testutil/sample/crosschain.go (1 hunks)
- x/crosschain/keeper/abci_test.go (9 hunks)
- x/crosschain/types/cctx.go (1 hunks)
Additional context used
Path-based instructions (4)
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/crosschain/types/cctx.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/abci_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/sample/crosschain.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (5)
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
92-93: Ensure backward compatibility with existing data.The addition of the
created_timestampfield should be backward compatible. Ensure that the field is optional to avoid issues with existing data that do not have this field populated.x/crosschain/types/cctx.go (1)
209-211: Ensure consistent timestamp usage.The
CreatedTimestampfield is populated with the Unix timestamp of the block header's time. Ensure that this timestamp is used consistently across the codebase to avoid discrepancies.x/crosschain/keeper/abci_test.go (2)
131-133: Ensure comprehensive test coverage forCreatedTimestamp.The
CreatedTimestampfield is correctly integrated into the test cases. Ensure that all edge cases are covered, such as scenarios where the timestamp might be missing or invalid.Also applies to: 156-158, 185-187, 214-216, 242-244, 265-267, 288-290, 311-313, 333-335
131-133: LGTM!The test cases are well-structured and cover the necessary scenarios for the
CreatedTimestampfield.Also applies to: 156-158, 185-187, 214-216, 242-244, 265-267, 288-290, 311-313, 333-335
testutil/sample/crosschain.go (1)
199-205: LGTM! Ensure the function usage in the codebase aligns with the new changes.The code changes are approved. The initialization and assignment of
createdAtto bothCreatedTimestampandLastUpdateTimestampenhance consistency in timestamp handling.However, ensure that all function calls to
Statusare verified to align with the new changes.
There are part of the workflow where the timestamp is not updated, we need to fix this. We can create an issue for this. |
|
@gartnera any blockers for merging this one ? |
53d233e to
804c789
Compare
804c789 to
53a902a
Compare
a14c076 to
e8c5f26
Compare
Description
Add
created_timestampto CCTX status. Closes #2611. The goal of this field is to be able to dolast_update_timestamp - created_timestampto be able to calculate CCTX latency.However, it seems that
last_update_timestampis not currently updated on a successful outbound:I would need to either:
last_update_timestampon any status changestatus_update_timestampwhich is always updated on status changePlease advise which option you prefer.
Update:
LastUpdateTimestampis now set inSetCrossChainTxso that it is always updated.Summary by CodeRabbit
New Features
CreatedTimestampfield to the transaction status, enabling improved tracking of when cross-chain transactions are created.Bug Fixes
Tests
CreatedTimestampfield for more accurate tracking and validation of cross-chain transaction statuses.