feat(zetacore)!: ensure cctx list is sorted by creation time #3548
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 pull request introduces timestamp indexing for cross-chain transactions. A new constant ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SetCrossChainTx
participant Store
participant TSIndexStore
participant Logger
Client->>SetCrossChainTx: Call SetCrossChainTx(tx)
SetCrossChainTx->>Store: Retrieve CCTX index bytes
Store-->>SetCrossChainTx: Return cctxIndexBytes
alt Error in retrieving index bytes
SetCrossChainTx->>Logger: Log error
SetCrossChainTx-->>Client: Return error
else No error
SetCrossChainTx->>TSIndexStore: Store (cctxIndexBytes + timestamp)
TSIndexStore-->>SetCrossChainTx: Acknowledge storage
SetCrossChainTx->>Store: Save CCTX with cctxIndexPrefix
Store-->>SetCrossChainTx: Confirm storage
end
sequenceDiagram
participant Client
participant CctxAll
participant TSStore
participant CctxStore
Client->>CctxAll: Request CCTX list with pagination
CctxAll->>TSStore: Paginate over timestamp index keys
TSStore-->>CctxAll: Return key list
loop For each key
CctxAll->>CctxStore: Retrieve CCTX data using key
CctxStore-->>CctxAll: Return transaction data
end
CctxAll-->>Client: Return aggregated CCTX list
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![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 (4)
x/crosschain/types/keys.go (1)
18-19: Remove duplicate comment.The comment "StoreKey defines the primary module store key" is duplicated from line 15.
- // StoreKey defines the primary module store key StoreKeyCreatedAtIndex = ModuleName + "-createdat"x/crosschain/keeper/cctx.go (2)
96-98: Consider moving constant to types package.The
TSIndexKeyconstant is used for store prefixes, similar to other keys defined in the types package. Consider moving it there for consistency.
115-119: Improve error handling.The error handling could be more informative by including the CCTX index in the error message.
- k.Logger(ctx).Error("unable to GetCCTXIndexBytes", "err", err) + k.Logger(ctx).Error("unable to GetCCTXIndexBytes", "err", err, "cctx_index", cctx.Index)x/crosschain/keeper/grpc_query_cctx.go (1)
59-66: Consider adding error handling for missing CCTX.While the implementation is correct, it might be helpful to log when a CCTX is not found in the store.
var cctx types.CrossChainTx cctxBytes := cctxStore.Get(value) + if cctxBytes == nil { + k.Logger(ctx).Error("cctx not found in store", "index", string(value)) + return nil + } if err := k.cdc.Unmarshal(cctxBytes, &cctx); err != nil { return err }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/crosschain/keeper/cctx.go(2 hunks)x/crosschain/keeper/grpc_query_cctx.go(2 hunks)x/crosschain/types/keys.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/keys.gox/crosschain/keeper/cctx.gox/crosschain/keeper/grpc_query_cctx.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-sim-import-export / sim
- GitHub Check: test-sim-fullappsimulation / sim
- GitHub Check: test-sim-nondeterminism / sim
- GitHub Check: test-sim-after-import / sim
- GitHub Check: lint
- GitHub Check: build-and-test
- GitHub Check: gosec
- GitHub Check: build-zetanode
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
x/crosschain/keeper/cctx.go (1)
120-123: LGTM! Well-implemented timestamp indexing.The use of big-endian encoding for timestamps ensures correct lexicographical sorting, and the comment explains this clearly.
x/crosschain/keeper/grpc_query_cctx.go (1)
48-49: LGTM! Clear and efficient store initialization.The store initialization is well-structured, with clear variable names that reflect their purpose.
Yeah I think it would be cleaner, it would definitely make pagination easier. But it does make the migration logic a bit harder. We could also just skip the migration and say this only applies to new data. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3548 +/- ##
===========================================
+ Coverage 64.72% 64.74% +0.02%
===========================================
Files 461 462 +1
Lines 31894 31960 +66
===========================================
+ Hits 20644 20694 +50
- Misses 10345 10358 +13
- Partials 905 908 +3
|
gartnera
force-pushed
the
cctx-list-createdat
branch
from
0b80548 to
19cd913
Compare
gartnera
changed the title
gartnera
changed the title
gartnera
force-pushed
the
cctx-list-createdat
branch
2 times, most recently
from
7dbf15d to
933ed00
Compare
Why? It will just be zero when first read? It will then immediately be incremented to 1. |
Shouldn't it be |
imo no since old CCTX will not be included in this index. |
Okay I think this test is good enough as well to guarantee this |
must also sort results to compare now
gartnera
force-pushed
the
cctx-list-createdat
branch
from
be9cce0 to
b43df25
Compare
gartnera
changed the title
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
gartnera
added
the
CONSENSUS_BREAKING_ACK
The
CctxAll()RPC is quite useless on live networks since the results are ordered bytssthenchainID.Add an index on theAdd an counter index which is incremented when a new CCTX is created. When listing all CCTX, use this index so that the CCTX are ordered by when they were created. You do have to specifycreated_timestampfield.pagination.reversed=trueright now, but I could also invert counter field the sort descending by default.This allows you to see the most recent CCTXs first.
Without this, it's quite annoying to calculate CCTX latency in the exporter and other places.
TODO:
sort_created_timestamp=trueparameter.I could also introduce a
ListRecentCctx(lookbackRange)which just uses the nonces to look for recent CCTXs. I'm mostly just interested in having a way lookup recent CCTX. It would look very similar toListPendingCctx(). Update: you can't do that because it would miss inbounds.Also:
unorderedparameter to get the old behaviorSummary by CodeRabbit
New Features
Refactor