chore: low-risk code-quality cleanup across tBTC/beacon node - #4185
chore: low-risk code-quality cleanup across tBTC/beacon node#4185piotr-roslaniec wants to merge 11 commits into
Conversation
…ments Extract registerAllMetrics into per-type helpers (counters, wallet actions, histograms, gauges) to isolate responsibilities, document the two-phase map-populate-then-observe concurrency invariant once per helper, remove field-group comments that restated field names, and correct the stale system-metrics ticker comment (60s).
…mments The coordinationFailed variable was only ever set true in branches that return immediately, so the success-metrics guard was always taken; remove the variable and simplify the guard. Also drop track-narration comments in coordination_window_metrics.go that restated the following line.
- add named DepositKey type replacing the anonymous struct used for DepositSweepProposal.DepositsKeys across tbtc, tbtcpg and ethereum - extract movingFundsSafetyMarginChain interface shared by ValidateMovingFundsSafetyMargin and isWalletPendingMovingFundsTarget - switch ParseWalletActionType on WalletActionType iota constants - collapse three identical frequency-window guards into a single guard
- EstimateDepositsSweepFee wraps the real error (was formatting the zero-valued sweepMaxSize) when GetDepositSweepMaxSize fails - sync_machine wraps the WaitForBlockHeight error with %w so callers can inspect the root cause - rename fnLogger to taskLogger to match the established logger naming - fix two Chain interface doc comments to start with the method name - correct the tools.go comment to describe indirect-dependency pinning
Wrap the finalSigningGroup error with %w so callers can inspect the underlying cause instead of only the outer message.
- add and register clientinfo deposit-sweep proof-submission metric constants, mirroring the redemption ones, and replace the raw metric name strings in the SPV maintainer with them - remove the getGlobalMetricsRecorder passthrough and call getMetricsRecorder directly - trim variable comments that restated the variable names in parseDepositSweepTransactionInputs, keeping the vault constraint note
Rename the minority marshalling.go files to the majority marshaling spelling for consistency across packages (git mv, no code changes).
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesProtobuf serialization
Performance and proof metrics
tBTC coordination and data flow
Error and documentation cleanup
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
The movingFundsSafetyMarginChain interface was inserted between the function's doc comment and its declaration, detaching the doc. Move the interface above the doc comment so it attaches again.
All five pinned modules are direct requires in go.mod, not indirect; describe them by what they actually are (build-time-only).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/beacon/dkg/marshaling.go`:
- Line 76: Validate protobuf member indices against group.MaxMemberIndex before
every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go
lines 76-76, reject pbThresholdSigner.MemberIndex before assigning
ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map
key before converting it. Preserve the existing error-handling flow for invalid
input.
In `@pkg/beacon/dkg/result/marshaling_test.go`:
- Around line 34-62: Assert the result of pbutils.RoundTrip in
TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it
fails; apply the same change to the claim-signature round-trip test in
pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing
fuzz setup and message construction.
In `@pkg/tbtcpg/deposit_sweep.go`:
- Around line 605-607: Update the error wrapping in the deposit sweep flow
around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the
underlying error for errors.Is and errors.As while keeping the existing context
message and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82ec20eb-1390-462e-9ffb-68d4184fb117
📒 Files selected for processing (28)
pkg/beacon/dkg/marshaling.gopkg/beacon/dkg/marshaling_test.gopkg/beacon/dkg/result/marshaling.gopkg/beacon/dkg/result/marshaling_test.gopkg/beacon/registry/marshaling.gopkg/beacon/registry/marshaling_test.gopkg/clientinfo/performance.gopkg/maintainer/spv/deposit_sweep.gopkg/maintainer/spv/deposit_sweep_test.gopkg/maintainer/spv/redemptions.gopkg/maintainer/spv/redemptions_test.gopkg/protocol/inactivity/marshaling.gopkg/protocol/inactivity/marshaling_test.gopkg/protocol/state/sync_machine.gopkg/tbtc/coordination.gopkg/tbtc/coordination_window_metrics.gopkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtc/dkg.gopkg/tbtc/marshaling.gopkg/tbtc/marshaling_test.gopkg/tbtc/moving_funds.gopkg/tbtc/wallet.gopkg/tbtcpg/chain.gopkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/internal/test/marshaling.gopkg/tbtcpg/redemptions.gotools.go
💤 Files with no reviewable changes (1)
- pkg/tbtc/coordination_window_metrics.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/beacon/dkg/marshaling.go`:
- Line 76: Validate protobuf member indices against group.MaxMemberIndex before
every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go
lines 76-76, reject pbThresholdSigner.MemberIndex before assigning
ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map
key before converting it. Preserve the existing error-handling flow for invalid
input.
In `@pkg/beacon/dkg/result/marshaling_test.go`:
- Around line 34-62: Assert the result of pbutils.RoundTrip in
TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it
fails; apply the same change to the claim-signature round-trip test in
pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing
fuzz setup and message construction.
In `@pkg/tbtcpg/deposit_sweep.go`:
- Around line 605-607: Update the error wrapping in the deposit sweep flow
around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the
underlying error for errors.Is and errors.As while keeping the existing context
message and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82ec20eb-1390-462e-9ffb-68d4184fb117
📒 Files selected for processing (28)
pkg/beacon/dkg/marshaling.gopkg/beacon/dkg/marshaling_test.gopkg/beacon/dkg/result/marshaling.gopkg/beacon/dkg/result/marshaling_test.gopkg/beacon/registry/marshaling.gopkg/beacon/registry/marshaling_test.gopkg/clientinfo/performance.gopkg/maintainer/spv/deposit_sweep.gopkg/maintainer/spv/deposit_sweep_test.gopkg/maintainer/spv/redemptions.gopkg/maintainer/spv/redemptions_test.gopkg/protocol/inactivity/marshaling.gopkg/protocol/inactivity/marshaling_test.gopkg/protocol/state/sync_machine.gopkg/tbtc/coordination.gopkg/tbtc/coordination_window_metrics.gopkg/tbtc/deposit_sweep.gopkg/tbtc/deposit_sweep_test.gopkg/tbtc/dkg.gopkg/tbtc/marshaling.gopkg/tbtc/marshaling_test.gopkg/tbtc/moving_funds.gopkg/tbtc/wallet.gopkg/tbtcpg/chain.gopkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/internal/test/marshaling.gopkg/tbtcpg/redemptions.gotools.go
💤 Files with no reviewable changes (1)
- pkg/tbtc/coordination_window_metrics.go
🛑 Comments failed to post (3)
pkg/beacon/dkg/marshaling.go (1)
76-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate every protobuf member index before narrowing to
group.MemberIndex. Both fields cross from protobufuint32into theuint8domain, allowing wrapped or colliding member IDs.
pkg/beacon/dkg/marshaling.go#L76-L76: rejectpbThresholdSigner.MemberIndex > group.MaxMemberIndexbefore assigning it.pkg/beacon/dkg/marshaling.go#L90-L98: reject each oversizedGroupPublicKeySharesmap key before converting it.📍 Affects 1 file
pkg/beacon/dkg/marshaling.go#L76-L76(this comment)pkg/beacon/dkg/marshaling.go#L90-L98🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/beacon/dkg/marshaling.go` at line 76, Validate protobuf member indices against group.MaxMemberIndex before every uint32-to-group.MemberIndex conversion: in pkg/beacon/dkg/marshaling.go lines 76-76, reject pbThresholdSigner.MemberIndex before assigning ts.memberIndex; in lines 90-98, reject each oversized GroupPublicKeyShares map key before converting it. Preserve the existing error-handling flow for invalid input.pkg/beacon/dkg/result/marshaling_test.go (1)
34-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert successful fuzz round-trips. Both tests discard the only failure signal from
pbutils.RoundTrip, reducing them to panic smoke tests.
pkg/beacon/dkg/result/marshaling_test.go#L34-L62: callt.Fatal(err)when the result-signature round-trip fails.pkg/protocol/inactivity/marshaling_test.go#L33-L61: callt.Fatal(err)when the claim-signature round-trip fails.📍 Affects 2 files
pkg/beacon/dkg/result/marshaling_test.go#L34-L62(this comment)pkg/protocol/inactivity/marshaling_test.go#L33-L61🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/beacon/dkg/result/marshaling_test.go` around lines 34 - 62, Assert the result of pbutils.RoundTrip in TestFuzzDKGResultHashSignatureMessageRoundtrip and call t.Fatal(err) when it fails; apply the same change to the claim-signature round-trip test in pkg/protocol/inactivity/marshaling_test.go at lines 33-61. Preserve the existing fuzz setup and message construction.pkg/tbtcpg/deposit_sweep.go (1)
605-607: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant section with line numbers. sed -n '585,620p' pkg/tbtcpg/deposit_sweep.go # Find similar error-wrapping patterns in the file for consistency. rg -n 'fmt\.Errorf\(.*%[vw]' pkg/tbtcpg/deposit_sweep.goRepository: threshold-network/keep-core
Length of output: 1569
Wrap the sweep-size error with
%w. Returning%vdrops the underlying error, so callers can’t useerrors.Isorerrors.Ason this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtcpg/deposit_sweep.go` around lines 605 - 607, Update the error wrapping in the deposit sweep flow around GetDepositSweepMaxSize to use `%w` instead of `%v`, preserving the underlying error for errors.Is and errors.As while keeping the existing context message and return behavior.
DepositKey replaced the anonymous struct previously inlined as the element type of DepositSweepProposal.DepositsKeys. Go does not allow assigning an anonymous-struct-typed slice literal to a named-struct-typed slice field, so any code outside this module that constructs a DepositSweepProposal from the old anonymous struct shape fails to compile against the new type, even though every in-repo consumer was already updated. Document this on the DepositKey type so downstream consumers of this package are not surprised by a silent compile break on upgrade.
|
Updated the PR description's "Scope / follow-ups" section: it previously listed exported/interface signature changes as entirely out of scope for this PR, but |
Neither of this PR's stated behavior fixes had direct test coverage: every EstimateDepositsSweepFee table case used depositsCount > 0, so the branch calling GetDepositSweepMaxSize (and its corrected error-wrapping) was never reached, and the panicking LocalChain double would have crashed the suite had it ever been exercised. - add LocalChain.SetDepositSweepMaxSizeError to let tests configure that failure without a real chain implementation - add a depositsCount: 0 case asserting the wrapped error keeps the real underlying cause instead of the old zero-value formatting - add TestDepositSweepProofSubmissionCountersRegistered, mirroring TestJoinFailureAndOnChainCountersRegistered, asserting the three new deposit-sweep proof-submission counters are pre-registered and exported
|
Two issues from review are fixed and pushed: Exported API change not reflected in the PR description — Missing test coverage for the two behavior fixes — neither the corrected error-wrapping in No outstanding issues from this pass. |
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
2 findings fixed and pushed; no outstanding issues requiring follow-up.
2 participants
Summary
A focused, low-risk code-quality pass across the tBTC/beacon node. Almost all
changes are behavior-preserving cleanups (dedup, named types, dead-code removal,
error wrapping, naming/doc/comment consistency). Two are small, beneficial
functional fixes, called out under Behavior changes below. Net -40 lines.
Each touched package builds, vets, and passes its test suite (including the full
pkg/tbtcsuite).Changes
registerAllMetricsinto per-type helpers(counters / wallet-actions / histograms / gauges), documenting the two-phase
map-populate-then-observe concurrency invariant once per helper; remove
field-group comments that restated field names; fix a stale ticker comment.
coordinationFailedflag (both set-true branchesreturn early, so the end guard was always taken); introduce a named
DepositKeytype replacing the anonymous struct shared across tbtc/tbtcpg/ethereum; extract a shared
movingFundsSafetyMarginChaininterface; switchParseWalletActionTypeon iota constants; collapse three identicalfrequency-window guards; wrap the final-signing-group resolution error.
EstimateDepositsSweepFeeand the sync machine (was dropping / formatting thewrong value); rename
fnLogger->taskLoggerto match convention; fix twointerface doc comments to start with the method name.
(mirroring redemptions) and replace raw metric-name strings; drop the
getGlobalMetricsRecorderpassthrough wrapper; trim restating variablecomments.
marshallingfilenames to the majoritymarshalingspelling (git mv, no code changes); correct thetools.gocomment to accurately describe the pinned modules as build-time-only
dependencies (they are direct
go.modrequires, not indirect).Behavior changes
Two changes alter runtime behavior; both are intentional fixes rather than pure
cleanups:
EstimateDepositsSweepFeenow reports the real error cause. The previousmessage formatted the zero-value
sweepMaxSize(cannot get sweep max size: [0]), dropping the actual error; it now wrapserr.deposit_sweep_proof_submissions_{total,success_total,failed_total}counterswere previously incremented but never registered as observers, so they were
never exported. Registering them in
registerCounterMetricsmakes themobservable for the first time.
Scope / follow-ups
This PR intentionally covers only safe-to-moderate cleanups. Deliberately left
out (larger blast radius, need dedicated review):
GetMovingFundsParameters),node.gomonoliths,
pkg/tbtcpackage layout, metrics DI),One accepted exception to the above:
DepositSweepProposal.DepositsKeys(see the tbtc bullet under Changes) changes element type from an
anonymous struct to the new named
DepositKeytype. Every in-repo consumerwas updated, so
go build ./...is clean, but this is a realsource-compatibility break for any code outside this module that constructs
a
DepositSweepProposaldirectly from the old anonymous-struct literal —that code will fail to compile against the new type. This is flagged on the
DepositKeygodoc as a heads-up for downstream consumers. It's a trivial,low-risk adaptation and doesn't need dedicated review, but it is a signature
change, not merely an internal cleanup, so it doesn't fit the "deliberately
left out" bucket above without this caveat.
Testing
go build ./...cleango testgreen for every touched package (clientinfo, tbtc, tbtcpg,protocol, maintainer/spv, beacon/{registry,dkg,dkg/result}, protocol/inactivity)
Summary by CodeRabbit
New Features
DepositKeytyping for deposit sweep proposals.Bug Fixes
Tests