docs(tbtcpg,spv): document wallet-action package roles#4189
docs(tbtcpg,spv): document wallet-action package roles#4189piotr-roslaniec wants to merge 1 commit into
Conversation
Neither pkg/tbtcpg nor pkg/maintainer/spv had a package doc, and nothing stated that they are the two halves of the wallet-action domain (tbtcpg = proposal generation, spv = proof maintenance) or that their mirrored file names are intentional. Add package docs stating each role and the mirror convention.
📝 WalkthroughWalkthroughAdds the ChangestBTC proposal generation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProposalGenerator
participant ProposalTask
Caller->>ProposalGenerator: Generate(request)
ProposalGenerator->>ProposalTask: Run(request)
ProposalTask-->>ProposalGenerator: proposal, ok, error
ProposalGenerator-->>Caller: proposal or aggregated error/no-op
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/tbtcpg/tbtcpg.go (1)
144-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not swallow task errors when another task returns no result.
If one task fails transiently and a later supported task returns
(ok=false, err=nil), this condition returnsNoopProposalinstead of propagating the error. That bypasses the coordination retry path and can permanently skip a valid action for the window. Return the aggregated error whenever no proposal was generated andtaskErrorsis non-empty.Proposed fix
- if len(taskErrors) > 0 && len(taskErrors) == len(request.ActionsChecklist)-len(unsupportedActions) { + if len(taskErrors) > 0 {🤖 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/tbtcpg.go` around lines 144 - 164, The no-op proposal path currently requires every supported task to fail before propagating errors, so it swallows errors when another task returns no result. Update the condition before the “all proposal tasks completed without result” log to return the aggregated task error whenever no proposal was generated and taskErrors is non-empty, regardless of other task outcomes; preserve the existing error aggregation and no-op behavior when taskErrors is empty.
🤖 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.
Outside diff comments:
In `@pkg/tbtcpg/tbtcpg.go`:
- Around line 144-164: The no-op proposal path currently requires every
supported task to fail before propagating errors, so it swallows errors when
another task returns no result. Update the condition before the “all proposal
tasks completed without result” log to return the aggregated task error whenever
no proposal was generated and taskErrors is non-empty, regardless of other task
outcomes; preserve the existing error aggregation and no-op behavior when
taskErrors is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a99a71d-e5e1-4243-95ce-ffe3c215dbb0
📒 Files selected for processing (2)
pkg/maintainer/spv/spv.gopkg/tbtcpg/tbtcpg.go
Summary
Neither
pkg/tbtcpgnorpkg/maintainer/spvhad a package doc comment, andnothing documented that they are the two halves of the tBTC wallet-action
domain:
pkg/tbtcpggenerates wallet-action proposals for the coordinator.pkg/maintainer/spvsubmits SPV proofs for the transactions those actionsproduce.
Their mirrored file names (
deposit_sweep.go,redemptions.go,moving_funds.go,moved_funds_sweep.go) are intentional but undocumented.This adds a package doc to each stating its role and the mirror convention, so
the split is discoverable and future code has a stated rule for where each
concern lives.
Docs-only; no code changes.
Verification
go build ./...andgo vet ./...clean;gofmtclean.Summary by CodeRabbit