chain: port over SubmitPackage from btcwallet - #11
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ports the SubmitPackage functionality from btcwallet to provide a temporary implementation for submitting transaction packages. The changes include adding a new BitcoindRPCClient in the chain package, updating dependencies, and exposing the new client through the test harness.
My review focuses on the new chain/bitcoind_client.go file. I've identified some dead code (unused constants and a struct field) and a discrepancy between a function's comment and its implementation regarding error handling. These seem to be artifacts from the porting process and should be cleaned up to improve code clarity and correctness.
| // If the active backend cannot relay packages, ErrUnimplemented is | ||
| // returned. |
There was a problem hiding this comment.
The function comment states that ErrUnimplemented is returned if the backend cannot relay packages. However, the implementation does not check for this case (e.g., by checking for btcjson.ErrRPCMethodNotFound from the RPC response) and therefore does not return chain.ErrUnimplemented. This discrepancy between documentation and implementation can lead to incorrect error handling by callers. Given this is a temporary port, removing the comment is the simplest fix to align documentation with behavior.
| const ( | ||
| // defaultSubmitPackageMaxTries is the default maximum number of tries | ||
| // for submitpackage RPC calls. | ||
| defaultSubmitPackageMaxTries = 60 | ||
| // defaultSubmitPackageRetryWait is the default wait time between | ||
| // submitpackage RPC call retries. | ||
| defaultSubmitPackageRetryWait = 1 * time.Second | ||
| ) |
There was a problem hiding this comment.
The constants defaultSubmitPackageMaxTries and defaultSubmitPackageRetryWait are defined but not used. This appears to be dead code copied over during porting. To avoid confusion, it should be removed.
PR Review: Port SubmitPackage from btcwalletThanks for this contribution! This is a clean port that adds important package relay functionality. Here's my detailed review: ✅ Positive Aspects
🔍 Issues & RecommendationsCRITICAL: Missing Test CoverageAccording to the repository development guidelines, the project strives for near 90% test coverage. This PR adds 135 lines of new code in chain/bitcoind_client.go with zero test coverage. Required Actions:
Code Style Issues
Documentation IssuesThe SubmitPackage comment mentions "The returned slice holds the txids" but the function returns *btcjson.SubmitPackageResult, not a slice. Update documentation to clarify what fields the result contains. Potential Issues
📋 Testing ChecklistBefore merging, ensure:
🎯 SummaryThis is a solid foundation, but tests are mandatory per project guidelines. The code quality is good overall with minor style issues. Once tests are added and the issues above are addressed, this will be ready to merge. Recommendation: Request changes for test coverage before approval. |
9015837 to
0e73ad8
Compare
0e73ad8 to
ca3d734
Compare
Code Review: SubmitPackage ImplementationThank you for porting over the SubmitPackage functionality from btcwallet! This is a solid implementation with clear purpose. Below are my findings: Strengths
Issues and Suggestions1. Missing Test Coverage (HIGH PRIORITY)Per CLAUDE.md, you must strive for near 90% test coverage where practical. This PR adds 123 lines of new code but no tests. Required tests:
Action: Add chain/bitcoind_client_test.go before merging. 2. Typo in Comment (line 59)Current: "Prepare the hex encoded transactions to that we'll add the request." Should be: "Prepare the hex encoded transactions that we'll add to the request." 3. Variable Naming Inconsistency (line 71)The variable "ch" is confusing—it suggests "channel" but represents a transaction. Use "parent" or "tx" for clarity: for i, parent := range parents {
h, err := toHex(parent)
// ...
}4. Error Message ConsistencyLines 51-52 and 56 use errors.New with inline error messages that bypass the nolint directive's intent. Consider defining sentinel errors at package level: var (
ErrNoParents = errors.New("submitpackage: need at least one parent txn")
ErrNoChild = errors.New("submitpackage: child txn not defined")
)This would remove the need for //nolint:err113 and improve testability. 5. Missing Context ParameterMany functions in the codebase accept context.Context as the first parameter (per development_guidelines.md). While the underlying RawRequest doesn't accept context, consider future-proofing: func (c *BitcoindRPCClient) SubmitPackage(ctx context.Context,
parents []*wire.MsgTx, child *wire.MsgTx,
maxFeeRateBTCPerVByte *float64) (*btcjson.SubmitPackageResult, error)This would allow for cancellation/timeout support if rpcclient adds context support later. 6. Code Organization - SpacingPer development_guidelines.md, code should be organized into logical stanzas with spacing. Consider adding blank lines between logical sections:
Security ConsiderationsNo obvious security issues. The function properly validates inputs and wraps errors without exposing sensitive data. Performance ConsiderationsGood preallocated slice capacity: make([]string, 0, len(parents)+1) Minor note: The toHex closure is fine, but you could hoist it outside the function if it's reused elsewhere in the package. Documentation CompletenessThe function comment on line 41 mentions "If the active backend cannot relay packages, ErrUnimplemented is returned" but this error is never created or returned in the function body. Either:
Before Merging:
Overall AssessmentThis is a clean, well-structured implementation. The main blocker is missing test coverage—please add tests before merging per project requirements. Commit Message Format Check: Properly formatted as "chain: port over SubmitPackage from btcwallet" |
Code Review: SubmitPackage ImplementationThank you for porting the SubmitPackage functionality! This PR adds important package relay capabilities. I've conducted a thorough review and found several issues that need addressing. While some concerns overlap with previous reviews, I'm highlighting additional aspects specific to this codebase's standards. Critical Issues1. Missing Test Coverage ⛔Per CLAUDE.md: "Strive for near 90% test coverage where practical." This PR adds 123 lines of production code with zero test coverage. Before merging, you MUST add:
Action Required: Run Code Quality Issues2. Comment Typo (line 59)- // Prepare the hex encoded transactions to that we'll add the request.
+ // Prepare the hex encoded transactions that we'll add to the request.3. Variable Naming (line 71)The variable name - for i, ch := range parents {
- h, err := toHex(ch)
+ for i, parent := range parents {
+ h, err := toHex(parent)4. Comment Typo (line 116)- // Unmarshall the response.
+ // Unmarshal the response.5. Code Spacing (multiple locations)Per
Example improvement for lines 59-69: // Prepare the hex encoded transactions that we'll add to the request.
toHex := func(tx *wire.MsgTx) (string, error) {
var buf bytes.Buffer
if err := tx.Serialize(&buf); err != nil {
return "", err
}
return hex.EncodeToString(buf.Bytes()), nil
}
// Serialize all parent transactions first, then the child.
rawTxs := make([]string, 0, len(parents)+1)Design and Documentation Issues6. Incomplete Documentation (line 41-42)The function comment states: "If the active backend cannot relay packages, ErrUnimplemented is returned." However, this error is never returned in the implementation. Either:
7. Error Handling Pattern (lines 50-56)Using var (
ErrNoParents = errors.New("submitpackage: need at least one parent txn")
ErrNoChild = errors.New("submitpackage: child txn not defined")
)This enables callers to use 8. Missing Fee Rate ValidationThe function accepts if maxFeeRateBTCPerVByte != nil && *maxFeeRateBTCPerVByte <= 0 {
return nil, fmt.Errorf("maxFeeRateBTCPerVByte must be positive: %f",
*maxFeeRateBTCPerVByte)
}9. Context Parameter MissingMost functions in Lightning Labs codebases accept func (c *BitcoindRPCClient) SubmitPackage(ctx context.Context,
parents []*wire.MsgTx, child *wire.MsgTx,
maxFeeRateBTCPerVByte *float64) (*btcjson.SubmitPackageResult, error)You can add a TODO comment explaining why it's not used yet, but having the parameter enables future context propagation without breaking API changes. Commit Organization ObservationPer CLAUDE.md: "Prefer small, atomic commits that build independently." Consider whether dependency updates (
While not strictly required for this PR, it's a best practice for future contributions. Pre-Merge ChecklistBefore this PR can be merged:
Strongly Recommended:
Positive Aspects
SummaryThis is a functional implementation that adds valuable package relay capabilities, but it needs tests and code style improvements before merging. The biggest blocker is the complete absence of test coverage—this is non-negotiable per project standards. Recommendation: Request changes for missing tests, style issues, and documentation clarification. |
2 participants
No description provided.