Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change introduces a new CLI flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| fmt.Println() // Add spacing between TSS entries | ||
| } | ||
| fmt.Printf("=== TSS %d of %d ===\n", i+1, len(tssHistoryRes.TssList)) | ||
| fmt.Printf("=== TSS %d of %d ===\n", i+1, len(tssList)) |
There was a problem hiding this comment.
Misleading TSS header when filtering by tss-number
Low Severity
When --tss-number is used, tssList is replaced with a single-element slice, so the header always prints === TSS 1 of 1 === regardless of which TSS was selected. For example, --tss-number 3 on a 5-entry list shows "TSS 1 of 1" instead of "TSS 3 of 5", losing both the requested position and total count context.
Additional Locations (1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/zetatool/cli/tss_balances.go`:
- Around line 173-185: The display counter should show the original total TSS
count even after filtering; before the block that mutates tssList when tssNumber
> 0, capture the original length (e.g., originalCount := len(tssList)), then
after potentially reducing tssList use that originalCount in the print statement
inside the loop (replace len(tssList) in the fmt.Printf call with originalCount)
so flags like --tss-number 3 will print "TSS 3 of N" correctly; update
references to tssNumber, tssList, and the fmt.Printf in the loop accordingly.
- Around line 167-171: The client is re-sorting tssHistoryRes.TssList by
FinalizedZetaHeight even though the server already returns entries sorted;
remove the redundant sort.Slice block (the anonymous func comparing
tssList[i].FinalizedZetaHeight and tssList[j].FinalizedZetaHeight) so the code
simply uses tssHistoryRes.TssList as returned, relying on the server-side
ordering and eliminating dead/duplicative logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 103935de-6c7c-4535-9c85-d9f88520d883
📒 Files selected for processing (1)
cmd/zetatool/cli/tss_balances.go
| // Sort TSS list by finalized zeta height ascending (oldest first, so TSS 1 = oldest, TSS N = latest) | ||
| tssList := tssHistoryRes.TssList | ||
| sort.Slice(tssList, func(i, j int) bool { | ||
| return tssList[i].FinalizedZetaHeight < tssList[j].FinalizedZetaHeight | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test cases related to TSS history sorting
rg -n "FinalizedZetaHeight.*0|TssHistory|tssList" --type=go -g '*_test.go'Repository: zeta-chain/node
Length of output: 7529
🏁 Script executed:
#!/bin/bash
# Find sample.TssList implementation
rg -n "func.*TssList" --type=go -A 10Repository: zeta-chain/node
Length of output: 1323
🏁 Script executed:
#!/bin/bash
# Check TSS protobuf definition
fd -e proto | xargs rg -l "message TSS" | head -5Repository: zeta-chain/node
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Check FinalizedZetaHeight in protobuf
rg -n "FinalizedZetaHeight" --type=protoRepository: zeta-chain/node
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Find the base Tss() sample function
rg -n "func Tss\(\)" --type=go -A 15Repository: zeta-chain/node
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Read the TSS protobuf definition
cat proto/zetachain/zetacore/observer/tss.proto | head -50Repository: zeta-chain/node
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Check how FinalizedZetaHeight is used in actual code (not tests)
rg -n "FinalizedZetaHeight" cmd/zetatool/cli/tss_balances.go -B 5 -A 5Repository: zeta-chain/node
Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Check how TSS entries are created/stored to see if FinalizedZetaHeight can be 0
rg -n "FinalizedZetaHeight.*=" --type=go -B 2 -A 2 | grep -v test | head -50Repository: zeta-chain/node
Length of output: 3031
🏁 Script executed:
#!/bin/bash
# Look for the server-side TssHistory query implementation mentioned in the review
fd -name "grpc_query_tss.go" | grep -v testRepository: zeta-chain/node
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check server-side TSS sorting logic
rg -n "TssHistory|sort.*TSS" x/observer/keeper/grpc_query_tss.go -A 10 -B 2Repository: zeta-chain/node
Length of output: 815
🏁 Script executed:
#!/bin/bash
# Check how TSS entries are created and stored (non-test code)
rg -n "TSS\{" --type=go -B 3 -A 8 | grep -v test | head -80Repository: zeta-chain/node
Length of output: 4874
🏁 Script executed:
#!/bin/bash
# Look for where TSS entries are created with potentially 0 FinalizedZetaHeight
rg -n "SetTSS|SetHistoricalTSS|append.*TSS" x/observer/keeper/ --type=go -B 2 -A 5 | head -60Repository: zeta-chain/node
Length of output: 3523
🏁 Script executed:
#!/bin/bash
# Check if there's logic for creating new/active TSS with unset FinalizedZetaHeight
rg -n "KeyGenZetaHeight|FinalizedZetaHeight" x/observer/keeper/msg_server*.go -B 3 -A 3Repository: zeta-chain/node
Length of output: 3272
🏁 Script executed:
#!/bin/bash
# Check actual TSS creation flow - when is SetTSSHistory called?
rg -n "SetTSSHistory|AppendTss" x/observer/keeper/ --type=go -B 3 -A 5Repository: zeta-chain/node
Length of output: 7307
🏁 Script executed:
#!/bin/bash
# Check what ctx.BlockHeight() returns and if it can be 0
rg -n "BlockHeight\(\)" x/observer/keeper/msg_server_vote_tss.go -B 2 -A 2Repository: zeta-chain/node
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Verify if any TSS history entries are ever created with FinalizedZetaHeight = 0
rg -n "FinalizedZetaHeight.*0|FinalizedZetaHeight\s*:" x/observer/keeper --type=go -B 2 -A 2 | grep -v test | grep -v "!= 0"Repository: zeta-chain/node
Length of output: 476
🏁 Script executed:
#!/bin/bash
# Check if there's any path where TSS could be stored with FinalizedZetaHeight = 0
rg -n "SetTSSHistory" --type=go -B 5 -A 2 | grep -v testRepository: zeta-chain/node
Length of output: 3420
🏁 Script executed:
#!/bin/bash
# Verify: does GetAllTSS only return historical TSS or include current active TSS?
rg -n "GetAllTSS|TSSHistoryKey|TSSKey" x/observer/keeper/tss.go -A 3Repository: zeta-chain/node
Length of output: 2148
🏁 Script executed:
#!/bin/bash
# Check the actual TSS query response to understand what data is returned
cat cmd/zetatool/cli/tss_balances.go | sed -n '160,180p'Repository: zeta-chain/node
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Confirm the server-side sorting happens before client receives data
cat x/observer/keeper/grpc_query_tss.go | sed -n '32,42p'Repository: zeta-chain/node
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about the intent of TSS numbering
rg -n "TSS.*1.*oldest|oldest.*TSS" cmd/zetatool/cli/ --type=go -B 2 -A 2Repository: zeta-chain/node
Length of output: 1833
Remove redundant client-side sorting—the server already returns sorted results.
The server-side TssHistory query (x/observer/keeper/grpc_query_tss.go:36–38) sorts TSS entries by FinalizedZetaHeight ascending before returning them. The client-side re-sort at lines 169–171 is unnecessary and violates the DRY principle.
Additionally, FinalizedZetaHeight is always set to ctx.BlockHeight() when a TSS is finalized (x/observer/keeper/msg_server_vote_tss.go:126), which is ≥ 1 for all actual transactions. The scenario of FinalizedZetaHeight == 0 cannot occur in practice, so the original ordering concern is not a functional risk.
Remove the client-side sort entirely to simplify the code and rely on the server's pre-sorted response:
Proposed fix: Remove redundant sort
// Sort TSS list by finalized zeta height ascending (oldest first, so TSS 1 = oldest, TSS N = latest)
tssList := tssHistoryRes.TssList
- sort.Slice(tssList, func(i, j int) bool {
- return tssList[i].FinalizedZetaHeight < tssList[j].FinalizedZetaHeight
- })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sort TSS list by finalized zeta height ascending (oldest first, so TSS 1 = oldest, TSS N = latest) | |
| tssList := tssHistoryRes.TssList | |
| sort.Slice(tssList, func(i, j int) bool { | |
| return tssList[i].FinalizedZetaHeight < tssList[j].FinalizedZetaHeight | |
| }) | |
| // TSS list is already sorted by finalized zeta height ascending (oldest first, so TSS 1 = oldest, TSS N = latest) | |
| tssList := tssHistoryRes.TssList |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/zetatool/cli/tss_balances.go` around lines 167 - 171, The client is
re-sorting tssHistoryRes.TssList by FinalizedZetaHeight even though the server
already returns entries sorted; remove the redundant sort.Slice block (the
anonymous func comparing tssList[i].FinalizedZetaHeight and
tssList[j].FinalizedZetaHeight) so the code simply uses tssHistoryRes.TssList as
returned, relying on the server-side ordering and eliminating dead/duplicative
logic.
There was a problem hiding this comment.
This is fine , there is no harm is sorting again
There was a problem hiding this comment.
@kingpinXD, understood — a defensive re-sort is indeed harmless and adds a degree of resilience against any future change to the server-side ordering. Happy to defer to your judgement here.
(◕ᴥ◕ )
🧠 Learnings used
Learnt from: gartnera
Repo: zeta-chain/node PR: 3228
File: zetaclient/orchestrator/orchestrator.go:388-401
Timestamp: 2024-11-27T22:01:49.732Z
Learning: When reviewing code changes in `zetaclient/orchestrator/orchestrator.go`, avoid suggesting refactoring that is unrelated to the current PR.
Learnt from: gartnera
Repo: zeta-chain/node PR: 3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like `cmd/zetae2e/init.go` in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 3627
File: e2e/runner/require.go:77-83
Timestamp: 2025-03-04T17:31:11.517Z
Learning: In the ZetaChain node repo, the ballot list returned from the ObserverClient.Ballots query (ballotsRes.Ballots) is already sorted by creation height, with the earliest ballots first.
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 3627
File: e2e/runner/require.go:77-83
Timestamp: 2025-03-04T17:31:11.517Z
Learning: In the ZetaChain node repo, the ballot list returned from the ObserverClient.Ballots query (ballotsRes.Ballots) is already sorted by creation height, with the earliest ballots first.
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 3503
File: e2e/runner/require.go:0-0
Timestamp: 2025-02-07T19:33:35.631Z
Learning: In e2e tests for the zeta-chain/node repository, minimize computation and avoid redundant checks that are already covered by unit tests to ensure faster execution. For example, ballot sorting is verified in unit tests, so e2e tests can safely assume the order.
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4064
File: cmd/zetae2e/local/solana.go:0-0
Timestamp: 2025-08-06T01:54:04.100Z
Learning: The `CheckSolanaTSSBalance()` method in e2e/runner has been refactored to not return an error. Instead, it uses internal assertions (require statements) to fail the test immediately if balance checks fail. This is part of a broader refactoring pattern in the E2E runner where balance check methods were changed from error-returning to assertion-based approaches.
| // Filter to a specific TSS if --tss-number is set | ||
| if tssNumber > 0 { | ||
| if tssNumber > len(tssList) { | ||
| return fmt.Errorf("TSS number %d out of range, only %d TSS entries available", tssNumber, len(tssList)) | ||
| } | ||
| tssList = []observertypes.TSS{tssList[tssNumber-1]} | ||
| } | ||
|
|
||
| for i, tss := range tssList { | ||
| if i > 0 { | ||
| fmt.Println() // Add spacing between TSS entries | ||
| } | ||
| fmt.Printf("=== TSS %d of %d ===\n", i+1, len(tssHistoryRes.TssList)) | ||
| fmt.Printf("=== TSS %d of %d ===\n", i+1, len(tssList)) |
There was a problem hiding this comment.
Display counter loses original TSS count after filtering.
After filtering to a single TSS at line 178, len(tssList) becomes 1. Consequently, line 185 prints "TSS 1 of 1" regardless of which TSS was requested. Users requesting --tss-number 3 would expect to see "TSS 3 of N" to understand the context of their selection.
Proposed fix to preserve original count
// Sort TSS list by finalized zeta height ascending (oldest first, so TSS 1 = oldest, TSS N = latest)
tssList := tssHistoryRes.TssList
sort.Slice(tssList, func(i, j int) bool {
return tssList[i].FinalizedZetaHeight < tssList[j].FinalizedZetaHeight
})
+ totalTSSCount := len(tssList)
// Filter to a specific TSS if --tss-number is set
+ selectedTSSNumber := 0
if tssNumber > 0 {
if tssNumber > len(tssList) {
return fmt.Errorf("TSS number %d out of range, only %d TSS entries available", tssNumber, len(tssList))
}
+ selectedTSSNumber = tssNumber
tssList = []observertypes.TSS{tssList[tssNumber-1]}
}
for i, tss := range tssList {
if i > 0 {
fmt.Println() // Add spacing between TSS entries
}
- fmt.Printf("=== TSS %d of %d ===\n", i+1, len(tssList))
+ displayNum := i + 1
+ if selectedTSSNumber > 0 {
+ displayNum = selectedTSSNumber
+ }
+ fmt.Printf("=== TSS %d of %d ===\n", displayNum, totalTSSCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/zetatool/cli/tss_balances.go` around lines 173 - 185, The display counter
should show the original total TSS count even after filtering; before the block
that mutates tssList when tssNumber > 0, capture the original length (e.g.,
originalCount := len(tssList)), then after potentially reducing tssList use that
originalCount in the print statement inside the loop (replace len(tssList) in
the fmt.Printf call with originalCount) so flags like --tss-number 3 will print
"TSS 3 of N" correctly; update references to tssNumber, tssList, and the
fmt.Printf in the loop accordingly.
There was a problem hiding this comment.
This is fine its an internal tool , i would rather have simplicity of code
There was a problem hiding this comment.
@kingpinXD, understood — simplicity is a fair trade-off for an internal tool. Happy to leave it as-is.
🧠 Learnings used
Learnt from: kingpinXD
Repo: zeta-chain/node PR: 4064
File: cmd/zetae2e/local/solana.go:0-0
Timestamp: 2025-08-06T01:54:04.100Z
Learning: The `CheckSolanaTSSBalance()` method in e2e/runner has been refactored to not return an error. Instead, it uses internal assertions (require statements) to fail the test immediately if balance checks fail. This is part of a broader refactoring pattern in the E2E runner where balance check methods were changed from error-returning to assertion-based approaches.
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
kingpinXD
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Description
--tss-numberto print the details for a specific tssHow Has This Been Tested?
Note
Low Risk
CLI-only change that filters/sorts displayed data; main risk is unexpected ordering/selection if callers relied on the previous implicit TSS list order.
Overview
Adds a new
--tss-numberflag tozetatool tss-balancesto print balances for only the Nth TSS entry.The command now sorts TSS history by
FinalizedZetaHeight(oldest first), validates the requested index, and updates help text/examples and output labeling to reflect the filtered list.Written by Cursor Bugbot for commit ccd0d1d. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Improvements