Minor Release v0.2.0 #43
Conversation
chore: move Address method into scripts.go
… into feat-orderbook
…on to handle data post submission (#27) * chore: refactored batch struct to save vouts,mergeTxFee * chore: refactored to read VoutIndices and MergeTxFee from Batch * chore: added function to handle data post submission * chore: refactored; added logic to retain requestIds while creating new batch * refactor * redirect to batchAndBroadcast if Tx is Not found added to SaveLatestBatch and getInAmounts * added test,constants,getDescendantFee func * added timeout contexts,err_checked TxFromBatch * moved RPCClient into client file * nitpicks * fixed value in < value out * fixed guardianWitness and guardianChangeSize typo --------- Co-authored-by: revantark <revantark@gmail.com>
…icks (#30) * chore: refactored batch struct to save vouts,mergeTxFee * chore: refactored to read VoutIndices and MergeTxFee from Batch * chore: added function to handle data post submission * chore: refactored; added logic to retain requestIds while creating new batch * refactor * redirect to batchAndBroadcast if Tx is Not found added to SaveLatestBatch and getInAmounts * added test,constants,getDescendantFee func * added timeout contexts,err_checked TxFromBatch * moved RPCClient into client file * nitpicks * fixed value in < value out * fixed guardianWitness and guardianChangeSize typo * added test for utxos and constructor for rpc * added New Tests, initial funding by simpleWallet * fixed fee estimation bug and update adjustfee function * removed MergeTxFee from batch;modified rpc constructor name --------- Co-authored-by: revantark <revantark@gmail.com>
WalkthroughThis pull request modifies multiple components across the codebase. In the README, a typographical error was introduced in the fee estimator description. In the Bitcoin files, address handling has been standardized, and several new methods and variables have been added for UTXO management, fee estimation, HTLC processing, RBF batch processing, and caching. New modules related to guardian-based transactions and orderbook smart contract bindings for Ethereum have been introduced along with corresponding tests and dependency updates in go.mod. Changes
Sequence Diagram(s)Guardian Wallet Transaction Signing FlowsequenceDiagram
participant User
participant GuardianWallet
participant GuardianClient
participant RemoteAPI
User->>GuardianWallet: Initiate transaction signing
GuardianWallet->>GuardianClient: Request to sign transaction
GuardianClient->>RemoteAPI: Send HTTP POST /sign request
RemoteAPI-->>GuardianClient: Return signed transaction data
GuardianClient-->>GuardianWallet: Provide signed transaction
GuardianWallet-->>User: Return transaction ID and signed tx
Orderbook Order Creation FlowsequenceDiagram
participant User
participant OrderbookWallet
participant OrderbookContract
participant EthNetwork
User->>OrderbookWallet: Request to create order
OrderbookWallet->>OrderbookContract: Pack order data & call CreateOrder
OrderbookContract->>EthNetwork: Process order transaction
EthNetwork-->>OrderbookContract: Return transaction receipt
OrderbookContract-->>OrderbookWallet: Send back transaction hash
OrderbookWallet-->>User: Return order creation tx hash
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 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
CodeRabbit Configuration File (
|
|
Update Readme to reflect all the new changes in the interfaces @raagan-u |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (3)
btc/htlc.go (3)
342-393: 🛠️ Refactor suggestionCheck
UserRedeemHTLCActionconsistency.
Executebranches on differentHTLCActions. Confirm thatUserRedeemHTLCActionindeed requires an SACP the same wayInstantRefundHTLCActiondoes, or if additional validation is needed.
464-506: 💡 Verification agent🧩 Analysis chain
Validate witness data for all inputs.
validateInstantRefundSACPchecks onlyTxIn[0]. If multiple inputs exist, ensure the witnesses in subsequent inputs aren’t overlooked.
🏁 Script executed:
#!/bin/bash rg -A5 "validateInstantRefundSACP"Length of output: 572
Enhanced Witness Validation Required:
The functionvalidateInstantRefundSACPcurrently validates witness data only for the first input (tx.TxIn[0]). While you correctly loop over all inputs to verify that txHashes match the provided UTXOs, the witness is only checked for index 0. If the transaction can contain multiple inputs, the witness data in subsequent inputs will remain unchecked. Please either iterate over all inputs to validate their witness elements (ensuring each has exactly 4 elements, with the first two having a 65-byte signature and matching refund script/control block as expected) or document that the transaction is always expected to have a single input.
398-436: 💡 Verification agent🧩 Analysis chain
Review off-by-one in locktime checks.
This logic calculates whether the timelock has expired
(utxo.Status.BlockHeight + Timelock - 1). Verify that you are not off by one block due to zero-based counting or chain nuance.
🏁 Script executed:
#!/bin/bash rg "need more blocks to refund"Length of output: 33
Let's run another shell script to pinpoint the definition of the timing verification logic (i.e. the
canRefundfunction) so we can confidently assess the off-by-one arithmetic.
🏁 Script executed:
#!/bin/bash rg "func canRefund" -n .Length of output: 121
Let's inspect the implementation of
canRefundaround its definition to better understand the locktime arithmetic. This will help confirm whether the off-by-one arithmetic is intentional or needs adjustment.
🏁 Script executed:
#!/bin/bash sed -n '505,525p' btc/htlc.goLength of output: 608
Locktime Arithmetic Off-By-One Issue in Refund Logic
There appears to be a potential off-by-one error in how the refund eligibility is calculated in the
canRefundfunction (around line 509 inbtc/htlc.go). Specifically:
For confirmed UTXOs, the condition checks whether
*utxo.Status.BlockHeight + timelock - 1 > currentTip
which implies that refunds become valid oncecurrentTipreaches(*utxo.Status.BlockHeight + timelock - 1).However, the subsequent computation for the number of additional blocks needed uses an extra
+1:
needMoreBlocks = *utxo.Status.BlockHeight + timelock - currentTip + 1
This arithmetic means that, for example, if an UTXO was confirmed at block 100 with a timelock of 10, refund eligibility would be triggered whencurrentTipequals 109, yet the calculation reports that 2 more blocks are needed whencurrentTipis 108 (since 100 + 10 - 108 + 1 equals 3, indicating an inconsistency).Additionally, the branch for unconfirmed UTXOs returns
timelock + 1, which might also require re-evaluation based on the intended semantics of the refund delay.Action Required:
Please verify whether the intended refund threshold should be atBlockHeight + Timelock - 1orBlockHeight + Timelock, and adjust the arithmetic accordingly. If the design expects the refund to be allowed only after a full timelock period has elapsed (i.e., currentTip ≥ BlockHeight + Timelock), then consider:
- Removing the
-1from the comparison, or- Eliminating the extra
+1in theneedMoreBlockscomputation.This change will ensure the refund logic accurately reflects the required block confirmations without an off-by-one error.
🧹 Nitpick comments (53)
go.mod (2)
37-37: New Indirect Dependency:github.com/davecgh/go-spew v1.1.1
The addition of this dependency can assist with improved pretty-printing during debugging. Verify that it is indeed required (likely picked up by another package) and consider running ago mod tidyto maintain a clean dependency tree.
58-58: New Indirect Dependency:github.com/pmezard/go-difflib v1.0.0
This dependency is useful for computing diffs, which can be beneficial in testing and debugging scenarios. Please confirm that its inclusion is necessary for your project’s functionality and that it does not inadvertently bloat your dependency graph.evm/orderbook_test.go (1)
16-23: Good utility function for secret generation.Using a cryptographically strong random generator is a robust approach for producing secrets. The panic on error is acceptable for tests, but consider logging or error returning if you reuse this helper in production contexts.
evm/orderbook.go (1)
170-172: Clarify appended 32-byte offset usage.Appending 32 bytes of zero before actual data (and setting a 16-bit value in the last bytes of that offset) may be a protocol requirement. Consider documenting why this offset is required or referencing where it's used downstream for clarity.
btc/store_test.go (1)
44-44: Updated function call to match new signature.The code now passes an empty string as the second parameter to
NewBatcherCache, reflecting the API change where the function now requires anidparameter. While the empty string works for tests, consider using a meaningful identifier even in test contexts.-cache = btc.NewBatcherCache(db, "", btc.CPFP) +cache = btc.NewBatcherCache(db, "test-cpfp-cache", btc.CPFP)btc/rbf_test.go (1)
83-83: Updated function call to match new signature.The code now passes an empty string as the second parameter to
NewBatcherCache, reflecting the API change. Similar to the other test file, consider using a meaningful identifier here.-cache = btc.NewBatcherCache(db, "", btc.RBF) +cache = btc.NewBatcherCache(db, "test-rbf-cache", btc.RBF)btc/fee.go (1)
58-83: Add test coverage forEstimateGuardianFee.
The calculation logic for virtual size and the retrieval of fee suggestions seems solid. However, consider adding dedicated unit tests to confirm correct behavior when:
- The extra segwit size or extra change size is large.
- The transaction has no segwit vs. all segwit inputs.
- Extreme fee levels (very high or very low).
btc/store.go (3)
61-65: Recipient conversion logic.
StoringRecipientviaEncodeAddress()is straightforward. Consider adding error handling ifspend.Recipientcannot be encoded for unexpected address types (e.g., testnet vs. mainnet).
136-139: Assigningidfields during deserialization.
Aligns well with new serialization logic. Make sure there's test coverage for scenarios whereIDorInvalidateIDis absent or malformed.
508-509: Usingid+strategyprefix in pending batch key.
Similar logic; no immediate concerns. Test different usage permutations to ensure no collisions in multi-tenant contexts.btc/rbf.go (4)
50-82: Robust fallback when transaction is not found.
Handling “tx not found” by retrieving a confirmed batch and recreating missing requests is a thoughtful approach. However, relying onstrings.Contains(err.Error(), "not found")can become brittle if the error message changes. Consider using typed errors if available.
116-116: Conditional check forErrTxInputsMissingOrSpent.
Handling the RBF flow depending on whether inputs are missing or spent is logical. Confirm that no other edge errors (e.g., insufficient fees) inadvertently match this condition.
170-170: Skipping “tx not found” with string check.
Similar concern as above regarding string-based error checks. For clearer error handling, consider typed or structured errors if possible.
595-606: UnusedgetChangeUTXOIndex.
Static analysis indicates this function is unused. Either remove it if not needed or add usages/tests if necessary.- func getChangeUTXOIndex(address btcutil.Address, txOuts []*wire.TxOut) (int64, int64, bool) { - // ... - } + // Remove or implement usage if required🧰 Tools
🪛 golangci-lint (1.62.2)
595-595: func
getChangeUTXOIndexis unused(unused)
btc/guardian/batch.go (3)
15-15: Format inconsistency in field declaration.There's an inconsistent tab being used instead of spaces in the MergeMap field declaration.
- MergeMap map[string]string + MergeMap map[string]string
19-21: Consider adding comments for constants.The constants lack documentation explaining their purpose and usage.
-const CoinbaseBatchID = "coinbase" -const GuardianWitnessSize = 267 -const GuardianChangeSize = 43 +// CoinbaseBatchID represents the ID used for coinbase transactions +const CoinbaseBatchID = "coinbase" +// GuardianWitnessSize represents the witness size in bytes for guardian signatures +const GuardianWitnessSize = 267 +// GuardianChangeSize represents the size in bytes for guardian change outputs +const GuardianChangeSize = 43
31-38: Enhance JSON marshaling implementation.The current JSON marshaling implementation is basic. Consider adding custom marshaling logic to handle specific field serialization requirements or validation.
Consider using a more robust approach:
// MarshalJSON implements the json.Marshaler interface func (b *Batch) MarshalJSON() ([]byte, error) { type Alias Batch return json.Marshal(&struct { *Alias }{ Alias: (*Alias)(b), }) } // UnmarshalJSON implements the json.Unmarshaler interface func (b *Batch) UnmarshalJSON(data []byte) error { type Alias Batch aux := &struct { *Alias }{ Alias: (*Alias)(b), } if err := json.Unmarshal(data, aux); err != nil { return err } return nil }This approach gives you more control over the marshaling process and allows for version handling in the future.
btc/batcher.go (1)
21-25:GuardianSpendWeightline has a TODO comment.Please remember to update the Guardian Script Size to the correct values or open a new issue to track this.
btc/wallet_test.go (2)
81-89: Multiple funding calls for the same address.Consider a helper loop or function to reduce duplication.
- _, err = localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer) - Expect(err).To(BeNil()) - ... + for i in {1..3} + do + _, err = localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer) + Expect(err).To(BeNil()) + done
619-620: Using a random address for coverage.Consider verifying final balance to strengthen test reliability.
btc/guardian/cache.go (5)
11-19: Consider documenting the interface methods.Adding doc comments for each interface method (e.g., what each method returns on "not found") can improve clarity for implementers and maintainers.
32-36: Validate the passed LevelDB instance.Consider checking if the
dbparameter is non-nil before assigning, to avoid potential panics ifNewCacheis accidentally called with a nil database.
38-49: Handle "not found" scenario more explicitly.Currently,
erris returned directly if retrieval fails. If the database returns a "not found" error, consider returning a sentinel error or a typed error so the caller can distinguish that case without relying onstrings.Contains.
67-78: Handle missing latest batch key distinctly.Similar to your approach in
ReadBatchByTxID, consider gracefully handling a missing batch key (e.g., by returning a specific error or a typed nil response) instead of a generic error.
84-90: Differentiate between parse failure and missing key.Combining parse and retrieval errors in a single return path can confuse callers. Consider returning a custom error or sentinel for "not found" vs. parse errors.
btc/guardian/wallet.go (5)
103-124: Refine the blacklisted address check.The code returns
"can not send to self"if the request destination matches the wallet's address. If multiple blacklisted addresses arise in the future, consider collecting them all before aborting so the caller gets a single combined error.
215-323: Large method warning: consider extracting sub-functions.
Sendis quite long and handles complexities like batch creation, merges, re-broadcast logic, and fee adjustments. Splitting into smaller methods (e.g.prepareBatch,broadcastTx, etc.) improves clarity and maintainability.
435-460: Convert fee touint64if negative fees cannot occur.
mergeTxFeereturns anint. If negative or extremely large fees are impossible, storing as an unsigned integer can be safer and clarify constraints. Otherwise, consider bounding checks for valid fee ranges.
534-633: Watch out for repeated transmissions inbatchAndBroadcast.If an error triggers a fallback path (like
w.batchAndBroadcastbeing called from multiple places), requests might be appended or re-sent. This could lead to duplicates if not thoroughly tested.
920-926: Remove or guardprettyPrintin production code.While helpful for debugging,
prettyPrintcan potentially flood logs or degrade performance when dealing with large data sets. Consider gating it behind a debug flag or removing it once stable.btc/guardian/wallet_test.go (4)
50-57: Unbounded goroutine for mining can complicate resource usage.The goroutine in
setupTestmines blocks indefinitely. For short test runs, this may be fine, but consider a mechanism to stop it or limit the block generation for deterministic test results and better resource management.
158-283: Potentially large random loops.The test "should be able to merge merge txs" uses random values for iteration counts. This can create flakiness or extended test durations. Consider random seeding with a fixed seed or limiting iteration to a smaller range for consistent CI runs.
310-323: Double-check “duplicate_request” test.The test attempts to send the same request ID "1_0" twice, but does not explicitly validate the outcome or compare to an expected error. If duplication is truly disallowed, consider adding an assertion that checks for a conflict error.
417-475: Confirm partial merges are not retried.“Invalidate the same request twice” test checks re-invalidation logic, but might benefit from verifying that once a merge request is invalidated, it can’t be re-added under another ID. Add an explicit assertion for that final state if desired.
btc/client.go (3)
347-353: Double-check JSON-RPC ID usage.The
IDfield is a string; other JSON-RPC clients often use numeric IDs. Ensure all downstream consumers expect a string ID, or consider using an integer if that offers clearer semantics.
362-378: Add basic field validation.The fields within
DescendantTransaction(e.g.,Vsize,Weight,DescendantCount, etc.) could benefit from sanity checks to prevent invalid or negative values.
387-393: Consider returning a pointer from the constructor.Returning a pointer to
BitcoinRPCClientcan be more idiomatic for instantiating clients, especially if you plan to manage internal state or references.-func NewBitcoinRPCClient(rpcUser string, rpcPass string, rpcURL string) BitcoinRPCClient { - return BitcoinRPCClient{ +func NewBitcoinRPCClient(rpcUser string, rpcPass string, rpcURL string) *BitcoinRPCClient { + return &BitcoinRPCClient{ RpcUser: rpcUser, ... } }btc/htlc.go (6)
37-51: Reassess expandingRawHTLCAction.Multiple specialized fields (
SACPTx,SACPSigAddAtIdx, etc.) were added to the same struct. If each field is only valid for certain actions (e.g., instantRefund vs. userRedeem), consider grouping them in a separate sub-struct or using derived types to reduce confusion.
105-107: Expand doc comments for usage.
GenerateRedeemSACPis introduced but the doc is brief. Clarify typical usage, any preconditions (like secret length or correctness), and constraints.
165-194: Validate secret correctness inGenerateRedeemSACP.Right now, no explicit check is in place for empty or invalid secrets. Adding a quick guard to ensure
secretis non-empty (and potentially the correct length) can prevent unwanted behavior.
203-208: Clarify multi-signature indexing."Signature is added at both 0th and 1st index" might confuse future maintainers. Briefly explain in the comments why the design requires two placeholders for multi-party signature logic.
439-455: RenamesigTxto clarify usage.
sigTxis used for the user-provided partial SACP in theRefundmethod. A more descriptive name would help, e.g.instantRefundPartialTx.
575-576: Confirm the leaf priority inhtlcLeaves.ToArray().The ordering is
[instantRefund, refund, redeem]. The merkle path depends on leaf ordering. If there's a required priority or known convention, highlight it in the docs.btc/wallet.go (5)
118-121: Ensure clarity around optionalRecipient.
SpendRequestnow has an optionalRecipient. Confirm that passing anilor unrelated address behaves consistently with standard spend flows.
140-156: Validate amounts in constructors.
NewSendRequestandNewSendRequestWithInvalidateIDdo not guard against negative or zero amounts. If the call site might pass invalid amounts, consider returning an error or panic to fail fast.
158-186: Streamline multi-return for ID, InvalidateTxID, MergeTxHex.Each method returns
(bool, string). Instead, you could return just the string and let callers check if it's empty, removing the extra boolean from the interface.
475-499: Refactor repetitive signature logic.
SignCoverUTXOsuses a loop to sign each UTXO individually, similar to standard segwit signing flows. Consider extracting shared logic if other parts duplicate it, to reduce risk of inconsistencies.
501-503: DocumentCoverUTXOSpendWeight.Currently just returns
SegwitSpendWeight. Add a short doc or rename the function to clarify that it's a fixed weight used for covering inputs.btc/guardian/guardian.go (1)
291-305: Use caller’s context instead ofcontext.Background().Both
GetGuardianPubkey(line 292) and the other methods inGuardianClientcould accept a caller-provided context to enable cancellation and deadline management.btc/guardian.go (4)
23-39: Fix the function name typo.The function name “NewGaurdianScript” is missing an “r.” Renaming it to “NewGuardianScript” will improve readability and consistency.
-func NewGaurdianScript(guardian, user *btcec.PublicKey) ([]byte, error) { +func NewGuardianScript(guardian, user *btcec.PublicKey) ([]byte, error) {
127-150: Be mindful of concurrent Cache writes.
Your approach for storing transactions ingc.Cacheis workable but ensure you guard it if you expect concurrent usage from multiple goroutines.
178-221: Consider removing leftover debug prints.Line 193 prints
txHex, which might leak transaction hex in logs or clutter console output in production.- fmt.Println("txHex", txHex)
496-502: Ensure that these placeholders do not cause unexpected issues.The
panic("Should never be called")can be replaced with a well-defined error return if you anticipate unexpected invocation scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
README.md(1 hunks)btc/batcher.go(4 hunks)btc/btc.go(1 hunks)btc/btc_suite_test.go(2 hunks)btc/client.go(2 hunks)btc/cpfp.go(5 hunks)btc/fee.go(2 hunks)btc/guardian.go(1 hunks)btc/guardian/batch.go(1 hunks)btc/guardian/cache.go(1 hunks)btc/guardian/guardian.go(1 hunks)btc/guardian/wallet.go(1 hunks)btc/guardian/wallet_test.go(1 hunks)btc/htlc.go(16 hunks)btc/htlc_test.go(3 hunks)btc/indexer.go(3 hunks)btc/rbf.go(19 hunks)btc/rbf_test.go(2 hunks)btc/scripts.go(2 hunks)btc/store.go(10 hunks)btc/store_test.go(1 hunks)btc/wallet.go(11 hunks)btc/wallet_test.go(25 hunks)evm/bindings/contracts/orderbook/orderbook.go(1 hunks)evm/orderbook.go(1 hunks)evm/orderbook_test.go(1 hunks)go.mod(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
btc/htlc_test.go (2)
Learnt from: Revantark
PR: catalogfi/blockchain#20
File: btc/htlc.go:0-0
Timestamp: 2025-03-14T06:24:02.262Z
Learning: The ineffectual assignment to `err` in the `GenerateInstantRefundSACP` function in `btc/htlc.go` was fixed in commit `e0b37abcf9c6f20c2e9e762b313da4c346926a4f`.
Learnt from: yanivas
PR: catalogfi/blockchain#12
File: btc/htlc.go:19-32
Timestamp: 2025-03-14T06:24:02.262Z
Learning: When generating unit tests for the `RedeemLeaf`, `InstantRefundLeaf`, and `toLeaf` functions in `btc/htlc.go`, ensure to cover various scenarios including valid inputs, empty inputs, and error cases.
btc/htlc.go (1)
Learnt from: Revantark
PR: catalogfi/blockchain#20
File: btc/htlc.go:0-0
Timestamp: 2025-03-14T06:24:02.262Z
Learning: The ineffectual assignment to `err` in the `GenerateInstantRefundSACP` function in `btc/htlc.go` was fixed in commit `e0b37abcf9c6f20c2e9e762b313da4c346926a4f`.
🪛 GitHub Actions: tests
btc/rbf_test.go
[error] 250-250: BatchWallet:RBF [It] should be able to spend multiple scripts and send to multiple parties: Expected length 5 but got length 6
btc/htlc_test.go
[error] 272-272: HTLC Wallet(p2tr) [It] should be able to initiate and redeem multiple HTLCs: Expected initID (unique id for the initiate action) is required for initiate action to be nil
btc/guardian/wallet_test.go
[error] 63-63: get account failed: Failed to retrieve account: no rows returned by a query that expected to return at least one row
btc/wallet_test.go
[error] 221-221: Wallets [It] should be able to send funds with overriding previous send request: Expected : bcrt1qecf5l0d0qzwemhe099aygl6cdhsmtqauu0pjay to equal : bcrt1qstfa9s4q5x52re2hhqrclfux5eurs8dl5fk5ja
🪛 golangci-lint (1.62.2)
btc/rbf.go
595-595: func getChangeUTXOIndex is unused
(unused)
btc/guardian.go
225-225: Error return value of updatedTx.Serialize is not checked
(errcheck)
231-231: Error return value of tx.Serialize is not checked
(errcheck)
🔇 Additional comments (98)
go.mod (1)
19-19: Direct Dependency Addition:github.com/stretchr/testify v1.10.0
This new direct dependency is a well-known testing library, which will help improve the robustness of your test suite. Please ensure that its version aligns with your existing test framework configurations and usage patterns.evm/bindings/contracts/orderbook/orderbook.go (4)
1-3: Auto-generated file confirmation.This file appears to be generated by a binding tool (e.g., abigen). Manual edits here may be overwritten. It's generally good practice to keep any manual logic separate from generated files to avoid losing changes.
47-61: Solid contract deployment flow.The deployment process consistently checks for ABI retrieval errors and deploys the contract if successful. Returning the contract wrapper along with the transaction ensures the caller can manage post-deployment tasks efficiently.
205-224: Transaction functions look correct.Both
CreateOrderandFillOrderappropriately bind to the respective contract methods, returning the transaction object for additional handling. Ensure that upstream callers handle post-transaction receipts and potential errors (e.g., out-of-gas, revert reasons).
260-300: Event iterator is well-structured.The iterative approach to fetching event logs carefully handles errors, subscription termination, and gracefully advances to new logs. This is a standard pattern for log retrieval loops.
evm/orderbook_test.go (2)
25-45: Comprehensive test data setup.Providing realistic values for chain names, asset fields, and destination addresses helps ensure accurate coverage. This ensures that both string-based and numeric-based fields are tested effectively.
47-75: Thorough packing/unpacking validation.The test verifies all fields, ensuring
PackCreateOrderandUnpackCreateOrderare symmetrical. This reduces the risk of partial or incorrect serialization.evm/orderbook.go (2)
125-147: Order creation workflow is well-structured.Obtaining a transactor, creating a contract instance, packing data, and invoking
CreateOrderis logically consistent. Downstream logic can handle transaction receipts.
182-184: Validation guard for unpacked values.Checking the length of
valuesensures the function fails early if the data is malformed or if the ABI changes. This helps avoid silent errors.btc/btc.go (1)
67-68: Simple formatting change looks good.This is a minor whitespace change adding a blank line after the Status field, likely for better readability and structure of the UTXO struct.
btc/indexer.go (2)
39-39: Good addition of the Size field to Transaction struct.This field will be useful for storing the transaction size in bytes, which is important for fee calculations and transaction management. This aligns with the broader changes related to fee estimation in this release.
375-385: Improved error handling logic.The modification enhances consistency in error reporting by defining
errMessagebefore the conditional check and using it for all cases. This is a good refactoring that ensures the error message is handled consistently regardless of which error condition is triggered.btc/rbf_test.go (1)
342-350:Details
❓ Verification inconclusive
Fix the failing test assertion.
The test is failing because it now expects at least 5 outputs instead of exactly 5, but line 250 still has a strict check for exactly 5 outputs. According to the pipeline error, the transaction now has 6 outputs but the test expects 5.
-Expect(tx.VOUTs).Should(HaveLen(5)) +Expect(len(tx.VOUTs) >= 5).Should(BeTrue())Also, since you've updated line 342 to check for at least 5 outputs, you should check whether line 350 (the wallet address output) should also be updated or removed if it's no longer being checked.
🏁 Script executed:
#!/bin/bash # Check the current implementation of VOUTs assertion in similar tests grep -A 5 "VOUTs.*HaveLen" --include="*.go" .Length of output: 71
Action: Review and update the strict output-length assertion to ensure consistency across the test.
It appears that while the check on lines 342–350 was updated to verify that there are at least 5 outputs, another assertion (around line 250) still enforces that exactly 5 outputs exist. Given the pipeline error (indicating that the transaction now has 6 outputs), please update the strict assertion in line 250 to match the new test logic. In addition, verify that the wallet address check in line 350 still correctly identifies the intended output—either update or remove it if it no longer reflects the wallet’s address.
To assist further, please run the following script to search for occurrences of the strict “HaveLen(5)” check across the Go files:
Once you’ve confirmed whether other tests require a similar update, please adjust accordingly.
🏁 Script executed:
#!/bin/bash rg "HaveLen\(5\)" --glob "*.go"Length of output: 209
Action Required: Update the strict output-length assertion in btc/rbf_test.go
It appears that while you've correctly updated the test at lines 342–350 to check that there are at least 5 outputs, line 250 still contains a strict assertion expecting exactly 5 outputs:
-Expect(tx.VOUTs).Should(HaveLen(5)) +Expect(len(tx.VOUTs) >= 5).Should(BeTrue())Next Steps:
- Update line 250 in btc/rbf_test.go to check for a minimum of 5 outputs instead of exactly 5 to reflect the new behavior.
- Reassess the wallet address check at line 350. Determine whether it still refers to the correct output now that the number of outputs can exceed 5. Either update the check accordingly or remove it if it no longer applies.
Please verify these changes ensure consistency with the overall test logic.
btc/scripts.go (2)
11-11: No concerns with the new import.
Theschnorrpackage appears to be necessary for taproot key serialization.
127-145: Confirm edge cases inHtlcScriptV2.
The function logic for assembling and constructing a Taproot address for an HTLC looks consistent. However, consider verifying behavior under edge cases, such as empty HTLC leaves. Ensure that tests confirm correct handling ifhtlcLeavesreturns no valid leaves and that all error conditions are exercised.Would you like me to outline a quick test to verify these edge cases?
btc/fee.go (1)
214-214: Approve simplified fee estimation check.
Removing network name checks in favor ofif f.url != ""is a more straightforward approach. Verifying thaturlis non-empty before fetching fee data is sufficiently robust for multiple environments.btc/store.go (13)
9-9: New import usage is fine.
The import ofbtcutilis consistent with the code changes for recipient/address handling.
30-30: Recipient field addition.
AddingRecipient stringtoserializableSpendRequestis coherent with the expanded logic for capturing a recipient address. No immediate issues noted.
35-39: Fields added toserializableSendRequest.
The new fieldsIDandInvalidateIDhelp clarify identity and invalidation semantics. Consider thoroughly documenting how these fields are intended to be used and tested.
73-74: Populating newRecipientfield.
The assignment is consistent with parsing logic above. Ensure that downstream usage is updated accordingly to handle this new field.
78-85: CapturingIDandInvalidateID.
The approach to read IDs fromsend.ID()andsend.InvalidateTxID()clarifies the final JSON structure. It correctly pairs with the newly introduced fields, no issues.
111-117: Parsing recipient address.
This handles deserialization gracefully, skipping when the recipient string is empty. Ensure coverage for invalid strings and consider verifying that the correct network is used for parsing.
126-126: PopulatingRecipientduring deserialization.
No immediate issues. Confirm that usage in the rest of the code doesn’t expect an uninitialized address object.
198-198: Updated constructor signature.
TheNewBatcherCache(db, id, strategy)signature provides an extra parameteridfor consistent key scoping. This should help maintain separation of different cache instances.
202-202: Storingidin key manager.
AttachingidtobatcherCacheKeyManagerensures all keys are scoped properly. Keep an eye on possible collisions if multiple IDs are used concurrently.
503-505: Incorporateid+strategyprefix in request index key.
This approach disambiguates keys well. Confirm that prefix concatenation doesn’t produce collisions for large or variedidstrings.
512-513: Batch key withid+strategy.
This is consistent with the pending batch key pattern. Good approach to unify naming.
516-517: Latest batch key withid+strategy.
Aligns with the rest of the updated scheme. Keep in mind to handle potential collisions or extremely longidvalues.
524-525: Pending request key withid+strategy.
Completes the set of modified prefixes for all relevant keys. Implementation is consistent across the board.btc/rbf.go (5)
8-9: New imports forstringsandtime.
These additions support error checks involving"not found"and context timeouts. No concerns here.
98-98: Updated method signature.
Renaming the parameter tonewRequestsclarifies the method’s intent. The logic to combineexistingRequestswithnewRequestsis consistent.
146-146: Appending missing and new requests.
Combining leftover requests from a confirmed batch with newly pending ones in a new RBF transaction is consistent with the fallback approach. No major issues.
268-273: Invocation ofremoveInvalidatedSendRequests.
Integrating the function within RBF batch creation effectively cleans up unneeded requests. Ensure that this step is documented so developers know that invalid requests are removed automatically.
642-642: Sufficient coverage forgetPendingFundingUTXOsusage.
The call togetPendingFundingUTXOsmerges pending and fresh UTXOs. Consider verifying that the returned UTXOs don’t introduce duplicates when they overlap with newly fetched ones.btc/btc_suite_test.go (2)
38-41: LGTM: Constants are properly aligned and formatted.The addition of the
BATCHER_GUARDIANmode follows the existing pattern of constant declarations.
52-53: LGTM: Mode parsing implementation is correct.The
parseModefunction has been properly updated to handle the newBATCHER_GUARDIANmode.btc/htlc_test.go (2)
300-304: LGTM: Added support for custom recipient in redemption.The addition of a
Recipientfield allows for flexible specification of where redeemed funds should be sent.
333-335: LGTM: Updated output verification to match enhanced redemption logic.The test now correctly expects two outputs: one for the recipient specified in the action and one for the wallet address.
btc/guardian/batch.go (1)
9-17: Good implementation of the Batch struct.The Batch struct provides a clean way to encapsulate transaction data with related metadata. The field documentation is helpful for understanding relationships.
btc/cpfp.go (6)
76-76: No concerns on the direct usage ofw.Address().
166-166: Consistent usage ofw.Address()for indexer calls.
287-287: Self-send logic is consistent.No issues with sending to
w.Address()when no other spend or send requests exist.
292-292: Safe to usew.Address()inbuildTransaction.The refactor aligns with the new address handling approach.
317-317:SignCoverUTXOsusage looks fine.The pass-through call is consistent with the new approach for covering UTXOs.
261-266:Details
✅ Verification successful
Potential duplication risk when re-appending UTXOs.
Please verify that appending new cover UTXOs to
spendUTXOsMap[w.Address().EncodeAddress()]won't re-introduce duplicates already filtered byremoveDoubleSpends.
🏁 Script executed:
#!/bin/bash rg -A 5 'spendUTXOsMap\[.Address'Length of output: 35
Below is a new step to gather additional context that will help confirm the risk of inadvertently reintroducing duplicate UTXOs. The initial search produced no output, so please run the following scripts to display the surrounding code and check the definition of
removeDoubleSpends:
Once you review the output from these commands, verify that the UTXOs filtered by
removeDoubleSpendsare not reintroduced when appending tospendUTXOsMap[w.Address().EncodeAddress()]. If the implementations ensure that duplicates are avoided, then the current approach is safe; otherwise, adjustments may be required.
🏁 Script executed:
#!/bin/bash # Print context from btc/cpfp.go around the lines in question sed -n '250,280p' btc/cpfp.goLength of output: 1193
🏁 Script executed:
#!/bin/bash # Locate the definition of removeDoubleSpends to review its duplicate filtering logic rg -n "func removeDoubleSpends" -t go .Length of output: 254
Let's further verify the implementation of
removeDoubleSpendsto confirm that it correctly filters out already-used UTXOs before they are re-appended. Please run the following script so we can inspect the function's logic:Once we see how the function handles duplicate UTXOs, we'll be able to determine if the current code effectively prevents reintroducing duplicates and update the review comment accordingly.
🏁 Script executed:
#!/bin/bash # Display the implementation of removeDoubleSpends to verify its duplicate filtering behavior sed -n '405,450p' btc/cpfp.goLength of output: 1404
Duplication Check Confirmed – Verify TxID Uniqueness
After inspecting the surrounding code and the implementation of
removeDoubleSpends, it appears that the risk of reintroducing duplicate UTXOs is effectively mitigated. The function builds a map of existing UTXOs based solely on theirTxID(from the current value ofspendUTXOsMap[w.Address().EncodeAddress()]) and filters out any cover UTXOs that share a TxID with those already present. When the filtered UTXOs are then appended back to the map, duplicates (by TxID) won’t be reintroduced.Notes:
- Ensure that using only the
TxIDis sufficient for identifying unique UTXOs in your application. If there’s any possibility that multiple distinct UTXOs from the same transaction (e.g., multiple outputs with different indices) need to be considered separately, additional uniqueness checks (like including the output index) might be necessary.btc/batcher.go (3)
239-239: RefactoredAddress()method to delegate tow.sw.Address().No issues, consistent with removing direct references to
w.address.
250-252:SignCoverUTXOspass-through is straightforward.Consider adding logging if needed, but this minimal approach is fine.
254-256:CoverUTXOSpendWeightpass-through is similarly minimal.No concerns here.
btc/wallet_test.go (18)
54-54:BATCHER_GUARDIANswitch case introduced.Good approach to unify enumerations for wallet configuration.
59-59: CPFP batcher cache creation.No issues, consistent with CPFP strategy usage.
61-64: New block forBATCHER_GUARDIANmode.Separate caches for "send" and "spend" is a sensible approach for multi-batching logic.
67-67: RBF batcher cache creation is consistent.
102-104: Deleting leftover database directories.No issues, but confirm that “tempDBDir_spend” exists in all scenarios before removing.
147-148: Refactored to useNewSendRequestconstructor.Enhances readability and ensures consistent parameter handling.
166-169: Output index assertions changed.Be mindful that reordering outputs might cause unpredictability if the wallet’s coin selection changes.
255-255: Skipping test for batcher modes in dust-limit scenario.
320-320: Skipping test for batcher modes in spending p2wpkh scenario.
544-588: New test for sending to different recipients in a P2WSH script.Ensures multi-recipients logic works with random addresses.
643-643: SettingRecipient: randomAddrin spend requests.No issues, but watch out for ordering or indexing in subsequent validations.
Also applies to: 653-653
665-669: Asserting three outputs and their addresses.Ensure stable output ordering if coin selection changes.
731-731: Skipping for batcher modes in status fetching test.
756-756: Skipping test for batcher modes in spend+send scenario.
975-975: Skipping test forBATCHER_GUARDIANin mixed SACP scenario.
1143-1143: Skipping forBATCHER_GUARDIANin fee-adjustment scenario.
1215-1215: Skipping forBATCHER_GUARDIANin p2tr SACP test.
1526-1526: AddedBATCHER_GUARDIANcase inassertSuccess.Consistent approach to finalize new mode tests.
btc/guardian/cache.go (3)
51-65: Great use of LevelDB batch operations.Your approach ensures atomicity when saving both the latest batch and the batch keyed by TxID. The code is straightforward and correct.
80-82: Fee storage approach looks good.Storing the integer fee as a string is a simple and effective approach, making the code easy to maintain.
92-102: Consistent keying approach for RequestVOUT.Your string-based key format for VOUTs is consistent with the rest of the codebase and aids debugging. The logic appears sound.
btc/guardian/wallet.go (1)
484-520:Details
❓ Verification inconclusive
Verify the logic merging unconfirmed requests.
getUnconfirmedRequestscross-referencespreviousBatchwithconfirmedBatchtransactions. Edge cases might arise if partially confirmed requests or double-spend attempts exist. Additional validations might be beneficial.
🏁 Script executed:
#!/bin/bash # Attempt to find partial references to unconfirmed requests usage outside of 'getUnconfirmedRequests'. rg -A 5 'unconfirmedRequests'Length of output: 29
Action: Revisit Merging Logic for Unconfirmed Requests
The current implementation of
getUnconfirmedRequestscorrectly cross-references the outputs in the confirmed batch against those in the previous batch by comparing the decoded script public keys. However, there’s a potential risk that edge cases—such as partially confirmed requests or double-spend attempts—aren’t fully covered by this logic. Our initial search for references to “unconfirmedRequests” outside this function produced no output, so no additional usage hints were identified. That said, the concern about handling these edge scenarios still stands.Please consider the following:
- Edge Case Validations: Evaluate if the current comparison strictly by script public key is sufficient. Should you encounter cases where a request is only partially confirmed, or if there’s any risk of double-spend attempts, additional validations might be warranted.
- Testing Scenarios: Add or update unit tests to simulate these edge conditions, ensuring that any potential partial confirmation or double-spending case is identified and handled appropriately.
Given these points, please manually verify that all potential edge cases are properly addressed by your implementation.
btc/client.go (2)
4-11: Imports look good.No immediate concerns. The newly introduced packages (
bytes,encoding/json,io,math,net/http) align with the added HTTP and JSON handling code.
380-384:Details
❓ Verification inconclusive
Guard credentials and consider secure storage.
The
RpcUserandRpcPassfields may contain sensitive data. Storing them in this struct can be risky if it persists. Confirm that you handle them securely (i.e., environment variables, secrets manager, restricted logs).
🏁 Script executed:
#!/bin/bash # Searching for any print or log usage of RpcPass rg "RpcPass"Length of output: 130
Secure Credentials Usage – Verification Outcome
After searching the codebase for occurrences of
RpcPass, the only usages we found are within the struct definition and in the call toreq.SetBasicAuth(b.RpcUser, b.RpcPass). No log or print statements that would inadvertently expose the secret were detected.
- The credentials are stored and later used only for setting the HTTP basic authentication header.
- There’s no evidence that these values are being logged or printed.
- Please confirm that the values for
RpcUserandRpcPassare supplied securely (for example, sourced from environment variables or a secrets manager) and are not inadvertently persisted or exposed elsewhere.btc/htlc.go (3)
11-11: Dependency on schnorr package looks fine.No issues with referencing
github.com/btcsuite/btcd/btcec/v2/schnorrfor taproot or schnorr-related logic.
25-25: New HTLC action name is consistent.
UserRedeemHTLCActionaligns well with existing action naming patterns (e.g., Initiate, Redeem, Refund).
287-336:Details
❓ Verification inconclusive
Confirm ordering of redeemed signatures.
When building partial or multi-party signatures in
instantRefund, ensure that external signers expect the same index ordering. Variations in signature index usage could cause invalid transactions.
🏁 Script executed:
#!/bin/bash rg "TestInstantRefund"Length of output: 22
Ensure Consistent Signature Indexing for Multi-party Refund Transactions
The current implementation in
instantRefundsets the redeemer’s signature in the witness array using the provided index (sigAddAtIdx). Before merging, please verify that all external signers are aligned with this ordering—where the 0th index is expected to be the redeemer’s signature and the 1st index is for the initiator’s signature. Since no tests (e.g., aTestInstantRefundtest case) were found to validate this assumption, manually confirm or add targeted tests to ensure consistency in signature index usage across multi-party transactions.btc/wallet.go (2)
230-236: Interface expansions look good.
SignCoverUTXOsandCoverUTXOSpendWeightare consistent with existing patterns for specialized wallet actions. No immediate concerns.
390-406:Details
❓ Verification inconclusive
Re-check coverage for appended requests.
When
extraSendRequestsare appended, you fetch more UTXOs. Make sure partial coverage or shortfall scenarios get tested thoroughly, ensuring no infinite recursion inspendAndSend.
🏁 Script executed:
#!/bin/bash rg "extraSendRequests"Length of output: 689
Action Required: Ensure Comprehensive Testing for Appended ExtraSendRequests
Please verify that your appended
extraSendRequestsare correctly handled across all edge cases. In particular:
- Confirm that scenarios with partial UTXO coverage or fee shortfalls are fully exercised in tests, both in
btc/wallet.go(lines 390-406) and in similar logic found inbtc/rbf.go.- Ensure that adding these extra send requests does not inadvertently trigger any infinite recursion in the
spendAndSendfunction (or similar recursive logic).btc/guardian/guardian.go (10)
1-2: Package declaration looks good.
18-22: Doc comment is concise and clear.
23-26: Struct definition is straightforward.
28-36: Account and signTxResponse definitions look good.
38-43: Constructor function appears appropriate.
45-54: Logic for account existence check is correct.
56-94: Account creation flow is solid.
133-180: Transaction signing routine looks good.
The approach to serialize the transaction, send it for signing, and re-deserialize is well-structured. Be aware of potential race conditions if multiple goroutines share the same transaction object.
245-271: GET request for account retrieval is properly handled.
96-131:Details
❓ Verification inconclusive
Confirm usage of root endpoint for Guardian pubkey.
Sometimes, callingc.urlwithout a specific sub-path might be incorrect depending on server configuration.Would you verify that the Guardian backend expects GET requests at the root path? For instance, you could run:
🏁 Script executed:
#!/bin/bash # Ensure root path returns a JSON with "public_key" field curl -i "$GUARDIAN_URL"Length of output: 66
Action Required: Verify Guardian Endpoint URL Usage
The current check didn’t yield a proper output because the URL variable was empty. Please confirm that the Guardian backend indeed expects GET requests at the root path (or if a sub-path is required, adjust accordingly). Manual verification with a valid URL is needed to ensure the endpoint returns a JSON object containing the"public_key"field.
- Ensure that the environment variable (or the value assigned to
c.url) is set to a valid Guardian backend URL.- Manually test the endpoint (e.g., via curl or Postman) and confirm that a GET request to that URL returns the expected JSON.
btc/guardian.go (11)
1-2: Package declaration is fine.
3-21: Imports appear consistent with usage.
41-52: Struct fields look fine; watch out for concurrency concerns with shared caches.
71-125: Account creation logic is correct.User address is decoded and parsed, and guardian pubkey is retrieved. No major issues spotted.
280-298: Fetching account list is straightforward.
300-318: Account retrieval logic is consistent.
320-353: Transaction retrieval approach is fine.
355-389: Bulk transaction retrieval is correctly decoding data.
391-421: Guardian pubkey fetch is appropriate.
423-495: Implementation of GuardianWallet is well-structured.Pay attention to the
panic("Should never be called")calls in methods likeSend– ensure the larger system never routes requests there.
504-668: MultiBatcherWallet usage is logically consistent.
No glaring concurrency or design issues identified.
| // removeInvalidatedSendRequests removes all send requests which are invalidated | ||
| func removeInvalidatedSendRequests(sendRequests []SendRequest) ([]SendRequest, error) { | ||
|
|
||
| // lets make sure no dup ids are there | ||
| ids := make(map[string]int) | ||
| for _, sr := range sendRequests { | ||
| ok, id := sr.ID() | ||
| if ok { | ||
| fmt.Println("id", id) | ||
| ids[id]++ | ||
| } | ||
| } | ||
|
|
||
| for _, count := range ids { | ||
| if count > 1 { | ||
| return nil, errors.New("duplicate ids found in send requests") | ||
| } | ||
| } | ||
|
|
||
| var removeIDs []string | ||
|
|
||
| for _, srI := range sendRequests { | ||
| found := false | ||
| ok, id := srI.InvalidateTxID() | ||
| if ok { | ||
| // find the id in the sendRequests | ||
| // if found remove the request from the sendRequests | ||
| // else remove the send request which has the invalidateID | ||
| for _, srJ := range sendRequests { | ||
| ok, srID := srJ.ID() | ||
| if ok && srID == id && srJ.Amount == srI.Amount { | ||
| removeIDs = append(removeIDs, srID) | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
| if ok && !found { | ||
| ok, id := srI.ID() | ||
| if !ok { | ||
| return nil, fmt.Errorf("send request has no id") | ||
| } | ||
| removeIDs = append(removeIDs, id) | ||
| } | ||
| } | ||
|
|
||
| // remove the requests from the sendRequests | ||
| for _, id := range removeIDs { | ||
| for i, sr := range sendRequests { | ||
| ok, srID := sr.ID() | ||
| if ok && srID == id { | ||
| sendRequests = append(sendRequests[:i], sendRequests[i+1:]...) | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return sendRequests, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New removeInvalidatedSendRequests function.
Good approach to handle conflicting or invalidated requests:
- Checking for duplicate IDs is helpful but might need more context on whether duplicates might exist in legit scenarios.
- Removing or skipping based on
InvalidateIDis sensible for partial invalidation logic.
Ensure thorough unit tests, especially with multiple invalidations or overlapping amounts.
Would you like an example test snippet to handle multiple edge cases here?
| ### Fee estimator | ||
|
|
||
| The fee estimator estimates network fees using various APIs. | ||
| The fee estimator gs network fees using various APIs. |
There was a problem hiding this comment.
Fix typographical error in fee estimator description.
The description of the fee estimator contains a typographical error: "gs" should be "estimates".
-The fee estimator gs network fees using various APIs.
+The fee estimator estimates network fees using various APIs.📝 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.
| The fee estimator gs network fees using various APIs. | |
| The fee estimator estimates network fees using various APIs. |
| Action: btc.InstantRefundHTLCAction, | ||
| HTLC: *aliceHTLC2, | ||
| SACPTx: sacpTx2, | ||
| SACPSigAddAtIdx: 1, | ||
| }, |
There was a problem hiding this comment.
Same issue: Add InitID to the second action.
Add the InitID field here as well to resolve the pipeline failure.
{
Action: btc.InstantRefundHTLCAction,
HTLC: *aliceHTLC2,
SACPTx: sacpTx2,
SACPSigAddAtIdx: 1,
+ InitID: "another-unique-id", // Add a unique identifier
},📝 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.
| Action: btc.InstantRefundHTLCAction, | |
| HTLC: *aliceHTLC2, | |
| SACPTx: sacpTx2, | |
| SACPSigAddAtIdx: 1, | |
| }, | |
| Action: btc.InstantRefundHTLCAction, | |
| HTLC: *aliceHTLC2, | |
| SACPTx: sacpTx2, | |
| SACPSigAddAtIdx: 1, | |
| InitID: "another-unique-id", // Add a unique identifier | |
| }, |
| Action: btc.InstantRefundHTLCAction, | ||
| HTLC: *aliceHTLC1, | ||
| SACPTx: sacpTx1, | ||
| SACPSigAddAtIdx: 1, | ||
| }, |
There was a problem hiding this comment.
Fix pipeline test failure in HTLC redemption.
The test is failing with: "Expected initID is required for initiate action to be nil"
An initID field appears to be missing from the RawHTLCAction for initiate actions. You need to add this field to resolve the pipeline failure.
{
Action: btc.InstantRefundHTLCAction,
HTLC: *aliceHTLC1,
SACPTx: sacpTx1,
SACPSigAddAtIdx: 1,
+ InitID: "some-unique-id", // Add a unique identifier
},📝 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.
| Action: btc.InstantRefundHTLCAction, | |
| HTLC: *aliceHTLC1, | |
| SACPTx: sacpTx1, | |
| SACPSigAddAtIdx: 1, | |
| }, | |
| Action: btc.InstantRefundHTLCAction, | |
| HTLC: *aliceHTLC1, | |
| SACPTx: sacpTx1, | |
| SACPSigAddAtIdx: 1, | |
| InitID: "some-unique-id", // Add a unique identifier | |
| }, |
| It("should be able to send funds with overriding previous send request", func() { | ||
| err = localnet.MineBitcoinBlocks(1, indexer) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| pk, err := btcec.NewPrivateKey() | ||
| Expect(err).To(BeNil()) | ||
| bobWallet, err := btc.NewSimpleWallet(pk, &chainParams, indexer, fixedFeeEstimator, feeLevel) | ||
| Expect(err).To(BeNil()) | ||
| bobAddr := bobWallet.Address() | ||
|
|
||
| req := []btc.SendRequest{ | ||
| btc.NewSendRequest("1", 100000, wallet.Address()), | ||
| btc.NewSendRequest("2", 100000, bobAddr), | ||
| } | ||
|
|
||
| txid, err := wallet.Send(context.Background(), req, nil, nil) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| var tx btc.Transaction | ||
| assertSuccess(wallet, &tx, txid, mode) | ||
|
|
||
| // lets create a random address | ||
| charlieAddr, err := randomP2wpkhAddress(chainParams) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| fmt.Println("charlieAddr", charlieAddr.EncodeAddress()) | ||
|
|
||
| req = []btc.SendRequest{ | ||
| btc.NewSendRequestWithInvalidateID("3", 100000, charlieAddr, "2", ""), | ||
| } | ||
|
|
||
| txid, err = wallet.Send(context.Background(), req, nil, nil) | ||
| Expect(err).To(BeNil()) | ||
| assertSuccess(wallet, &tx, txid, mode) | ||
|
|
||
| daveAddr, err := randomP2wpkhAddress(chainParams) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| // but change the id to 1 | ||
| req = []btc.SendRequest{ | ||
| btc.NewSendRequestWithInvalidateID("4", 100000, daveAddr, "1", ""), | ||
| } | ||
|
|
||
| txid, err = wallet.Send(context.Background(), req, nil, nil) | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| assertSuccess(wallet, &tx, txid, mode) | ||
|
|
||
| Expect(tx.VOUTs[1].ScriptPubKeyAddress).Should(Equal(daveAddr.EncodeAddress())) | ||
|
|
||
| fmt.Println("tx", tx.TxID) | ||
| fmt.Println("tx.vouts", wallet.Address().EncodeAddress()) | ||
|
|
||
| }) |
There was a problem hiding this comment.
Test is failing due to an address mismatch in pipeline logs.
Expected bcrt1qecf5l0... to equal bcrt1qstfa9s...
Please verify whether the override logic is generating a new address or if the test’s expected address is incorrect.
🧰 Tools
🪛 GitHub Actions: tests
[error] 221-221: Wallets [It] should be able to send funds with overriding previous send request: Expected : bcrt1qecf5l0d0qzwemhe099aygl6cdhsmtqauu0pjay to equal : bcrt1qstfa9s4q5x52re2hhqrclfux5eurs8dl5fk5ja
| func (b *BitcoinRPCClient) GetDescendants(ctx context.Context, txId string) (int64, error) { | ||
| verbose := true | ||
| // Create the JSON-RPC request | ||
| requestBody := RPCRequest{ | ||
| Jsonrpc: "1.0", | ||
| ID: "curltext", | ||
| Method: "getmempooldescendants", | ||
| Params: []interface{}{txId, verbose}, | ||
| } | ||
| jsonData, err := json.Marshal(requestBody) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("Error marshalling JSON: %w", err) | ||
| } | ||
|
|
||
| // Create a new HTTP request | ||
| req, err := http.NewRequestWithContext(ctx, "POST", b.RpcURL, bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("Error creating request: %w", err) | ||
| } | ||
|
|
||
| // Add headers | ||
| req.Header.Set("Content-Type", "text/plain") | ||
| req.SetBasicAuth(b.RpcUser, b.RpcPass) | ||
|
|
||
| // Send the request | ||
| client := &http.Client{} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("Error sending request: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Read response | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("Error reading response: %w", err) | ||
| } | ||
|
|
||
| var apiResponse struct { | ||
| Result map[string]DescendantTransaction `json:"result"` | ||
| Error interface{} `json:"error"` | ||
| ID string `json:"id"` | ||
| } | ||
|
|
||
| if err := json.Unmarshal(body, &apiResponse); err != nil { | ||
| return 0, fmt.Errorf("Error unmarshalling JSON: %w", err) | ||
| } | ||
|
|
||
| if len(apiResponse.Result) == 0 { | ||
| return 0, nil | ||
| } | ||
|
|
||
| transactions := make([]DescendantTransaction, 0, len(apiResponse.Result)) | ||
| for _, tx := range apiResponse.Result { | ||
| transactions = append(transactions, tx) | ||
| } | ||
|
|
||
| var sum float64 | ||
| for _, tx := range transactions { | ||
| sum += tx.Fees.Base | ||
| } | ||
|
|
||
| return int64(math.Round(sum * 100000000)), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle apiResponse.Error and HTTP status codes.
Currently, the code ignores apiResponse.Error in the JSON body and does not check resp.StatusCode. If the RPC call fails, you could lose critical error details. Add a check to surface these errors properly.
if apiResponse.Error != nil {
- // currently ignored
+ return 0, fmt.Errorf("RPC returned an error: %v", apiResponse.Error)
}
if resp.StatusCode != http.StatusOK {
return 0, fmt.Errorf("unexpected status code: %d", resp.StatusCode)
}📝 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.
| func (b *BitcoinRPCClient) GetDescendants(ctx context.Context, txId string) (int64, error) { | |
| verbose := true | |
| // Create the JSON-RPC request | |
| requestBody := RPCRequest{ | |
| Jsonrpc: "1.0", | |
| ID: "curltext", | |
| Method: "getmempooldescendants", | |
| Params: []interface{}{txId, verbose}, | |
| } | |
| jsonData, err := json.Marshal(requestBody) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error marshalling JSON: %w", err) | |
| } | |
| // Create a new HTTP request | |
| req, err := http.NewRequestWithContext(ctx, "POST", b.RpcURL, bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error creating request: %w", err) | |
| } | |
| // Add headers | |
| req.Header.Set("Content-Type", "text/plain") | |
| req.SetBasicAuth(b.RpcUser, b.RpcPass) | |
| // Send the request | |
| client := &http.Client{} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error sending request: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| // Read response | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error reading response: %w", err) | |
| } | |
| var apiResponse struct { | |
| Result map[string]DescendantTransaction `json:"result"` | |
| Error interface{} `json:"error"` | |
| ID string `json:"id"` | |
| } | |
| if err := json.Unmarshal(body, &apiResponse); err != nil { | |
| return 0, fmt.Errorf("Error unmarshalling JSON: %w", err) | |
| } | |
| if len(apiResponse.Result) == 0 { | |
| return 0, nil | |
| } | |
| transactions := make([]DescendantTransaction, 0, len(apiResponse.Result)) | |
| for _, tx := range apiResponse.Result { | |
| transactions = append(transactions, tx) | |
| } | |
| var sum float64 | |
| for _, tx := range transactions { | |
| sum += tx.Fees.Base | |
| } | |
| return int64(math.Round(sum * 100000000)), nil | |
| } | |
| func (b *BitcoinRPCClient) GetDescendants(ctx context.Context, txId string) (int64, error) { | |
| verbose := true | |
| // Create the JSON-RPC request | |
| requestBody := RPCRequest{ | |
| Jsonrpc: "1.0", | |
| ID: "curltext", | |
| Method: "getmempooldescendants", | |
| Params: []interface{}{txId, verbose}, | |
| } | |
| jsonData, err := json.Marshal(requestBody) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error marshalling JSON: %w", err) | |
| } | |
| // Create a new HTTP request | |
| req, err := http.NewRequestWithContext(ctx, "POST", b.RpcURL, bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error creating request: %w", err) | |
| } | |
| // Add headers | |
| req.Header.Set("Content-Type", "text/plain") | |
| req.SetBasicAuth(b.RpcUser, b.RpcPass) | |
| // Send the request | |
| client := &http.Client{} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error sending request: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| // Read response | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return 0, fmt.Errorf("Error reading response: %w", err) | |
| } | |
| var apiResponse struct { | |
| Result map[string]DescendantTransaction `json:"result"` | |
| Error interface{} `json:"error"` | |
| ID string `json:"id"` | |
| } | |
| if err := json.Unmarshal(body, &apiResponse); err != nil { | |
| return 0, fmt.Errorf("Error unmarshalling JSON: %w", err) | |
| } | |
| // Check for RPC error in the response | |
| if apiResponse.Error != nil { | |
| return 0, fmt.Errorf("RPC returned an error: %v", apiResponse.Error) | |
| } | |
| // Check HTTP status code | |
| if resp.StatusCode != http.StatusOK { | |
| return 0, fmt.Errorf("unexpected status code: %d", resp.StatusCode) | |
| } | |
| if len(apiResponse.Result) == 0 { | |
| return 0, nil | |
| } | |
| transactions := make([]DescendantTransaction, 0, len(apiResponse.Result)) | |
| for _, tx := range apiResponse.Result { | |
| transactions = append(transactions, tx) | |
| } | |
| var sum float64 | |
| for _, tx := range transactions { | |
| sum += tx.Fees.Base | |
| } | |
| return int64(math.Round(sum * 100000000)), nil | |
| } |
| // Optional ID of the send request | ||
| id string | ||
|
|
||
| // Amount to send | ||
| Amount int64 | ||
| // Recipient address | ||
| To btcutil.Address | ||
|
|
||
| // Invalidate the previous send request if it exists and make a direct send request | ||
| // instead of routing through middle script | ||
| invalidateID string | ||
|
|
||
| // MergeTxHex is the txid of the tx that is being merged | ||
| mergeTxHex string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit naming for added fields.
Fields like id, invalidateID, and mergeTxHex can be confusing without context. Document their usage and confirm no collisions with future expansions of SendRequest.
| // Creates a guardian client and caches the account for further use | ||
| type GuardianClient struct { | ||
| c *Client | ||
| account *Account | ||
| } | ||
|
|
||
| func NewGuardianClient(ctx context.Context, url string, userPubKey *btcec.PublicKey) *GuardianClient { | ||
| client := NewClient(url) | ||
| account, err := client.CreateAccountIfNotExists(ctx, userPubKey) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return &GuardianClient{ | ||
| c: client, | ||
| account: account, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning errors instead of panicking.
Using panic (line 283) in NewGuardianClient can terminate the program unexpectedly and hinder graceful error handling.
func NewGuardianClient(ctx context.Context, url string, userPubKey *btcec.PublicKey) *GuardianClient {
client := NewClient(url)
account, err := client.CreateAccountIfNotExists(ctx, userPubKey)
if err != nil {
- panic(err)
+ // Return nil and the error, or wrap and return an error
+ // return nil, fmt.Errorf("failed to create account: %w", err)
}
...
}Committable suggestion skipped: line range outside the PR's diff.
| func (c *Client) UpdateTransaction( | ||
| ctx context.Context, | ||
| accountAddr string, | ||
| prevTxID chainhash.Hash, | ||
| updatedTx *wire.MsgTx, | ||
| mergeTxs []string, | ||
| values []int64, | ||
| ) error { | ||
| var buf = new(bytes.Buffer) | ||
| updatedTx.Serialize(buf) | ||
| txHex := hex.EncodeToString(buf.Bytes()) | ||
|
|
||
| type UpdateTxRequest struct { | ||
| Address string `json:"address"` | ||
| TxHex string `json:"tx_hex"` | ||
| Values []int64 `json:"values"` | ||
| MergeTxs []string `json:"merge_txs"` | ||
| } | ||
| updateRequest := UpdateTxRequest{ | ||
| Address: accountAddr, | ||
| TxHex: txHex, | ||
| Values: values, | ||
| MergeTxs: mergeTxs, | ||
| } | ||
| jsonData, err := json.Marshal(updateRequest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "PUT", fmt.Sprintf("%s/transactions/%s", c.url, prevTxID.String()), bytes.NewBuffer(jsonData)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := c.httpClient.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| errMsg, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("update transaction failed to read error response: %w", err) | ||
| } | ||
| return fmt.Errorf("update transaction failed : %s", string(errMsg)) | ||
| } | ||
|
|
||
| var updatedTxResp UpdateTxRequest | ||
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | ||
| if err != nil { | ||
| return fmt.Errorf("update transaction failed to decode response: %w", err) | ||
| } | ||
|
|
||
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | ||
| if err != nil { | ||
| return fmt.Errorf("update transaction failed to decode response: %w", err) | ||
| } | ||
|
|
||
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | ||
| } |
There was a problem hiding this comment.
Check the return value of updatedTx.Serialize(buf).
Currently, the error from serialization is ignored, which can mask I/O or memory errors and potentially produce an invalid PUT request.
Apply this diff to handle the error:
func (c *Client) UpdateTransaction(
ctx context.Context,
accountAddr string,
prevTxID chainhash.Hash,
updatedTx *wire.MsgTx,
mergeTxs []string,
values []int64,
) error {
var buf = new(bytes.Buffer)
- updatedTx.Serialize(buf)
+ if err := updatedTx.Serialize(buf); err != nil {
+ return fmt.Errorf("failed to serialize updated transaction: %w", err)
+ }
txHex := hex.EncodeToString(buf.Bytes())
...📝 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.
| func (c *Client) UpdateTransaction( | |
| ctx context.Context, | |
| accountAddr string, | |
| prevTxID chainhash.Hash, | |
| updatedTx *wire.MsgTx, | |
| mergeTxs []string, | |
| values []int64, | |
| ) error { | |
| var buf = new(bytes.Buffer) | |
| updatedTx.Serialize(buf) | |
| txHex := hex.EncodeToString(buf.Bytes()) | |
| type UpdateTxRequest struct { | |
| Address string `json:"address"` | |
| TxHex string `json:"tx_hex"` | |
| Values []int64 `json:"values"` | |
| MergeTxs []string `json:"merge_txs"` | |
| } | |
| updateRequest := UpdateTxRequest{ | |
| Address: accountAddr, | |
| TxHex: txHex, | |
| Values: values, | |
| MergeTxs: mergeTxs, | |
| } | |
| jsonData, err := json.Marshal(updateRequest) | |
| if err != nil { | |
| return err | |
| } | |
| req, err := http.NewRequestWithContext(ctx, "PUT", fmt.Sprintf("%s/transactions/%s", c.url, prevTxID.String()), bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := c.httpClient.Do(req) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| errMsg, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to read error response: %w", err) | |
| } | |
| return fmt.Errorf("update transaction failed : %s", string(errMsg)) | |
| } | |
| var updatedTxResp UpdateTxRequest | |
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to decode response: %w", err) | |
| } | |
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to decode response: %w", err) | |
| } | |
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | |
| } | |
| func (c *Client) UpdateTransaction( | |
| ctx context.Context, | |
| accountAddr string, | |
| prevTxID chainhash.Hash, | |
| updatedTx *wire.MsgTx, | |
| mergeTxs []string, | |
| values []int64, | |
| ) error { | |
| var buf = new(bytes.Buffer) | |
| if err := updatedTx.Serialize(buf); err != nil { | |
| return fmt.Errorf("failed to serialize updated transaction: %w", err) | |
| } | |
| txHex := hex.EncodeToString(buf.Bytes()) | |
| type UpdateTxRequest struct { | |
| Address string `json:"address"` | |
| TxHex string `json:"tx_hex"` | |
| Values []int64 `json:"values"` | |
| MergeTxs []string `json:"merge_txs"` | |
| } | |
| updateRequest := UpdateTxRequest{ | |
| Address: accountAddr, | |
| TxHex: txHex, | |
| Values: values, | |
| MergeTxs: mergeTxs, | |
| } | |
| jsonData, err := json.Marshal(updateRequest) | |
| if err != nil { | |
| return err | |
| } | |
| req, err := http.NewRequestWithContext(ctx, "PUT", fmt.Sprintf("%s/transactions/%s", c.url, prevTxID.String()), bytes.NewBuffer(jsonData)) | |
| if err != nil { | |
| return err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := c.httpClient.Do(req) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| errMsg, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to read error response: %w", err) | |
| } | |
| return fmt.Errorf("update transaction failed : %s", string(errMsg)) | |
| } | |
| var updatedTxResp UpdateTxRequest | |
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to decode response: %w", err) | |
| } | |
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | |
| if err != nil { | |
| return fmt.Errorf("update transaction failed to decode response: %w", err) | |
| } | |
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | |
| } |
| func (gc *GuardianClient) updateTx(prevTxID chainhash.Hash, updatedTx *wire.MsgTx, mergeTxs []*wire.MsgTx, values []int64) error { | ||
| var buf = new(bytes.Buffer) | ||
| updatedTx.Serialize(buf) | ||
| txHex := hex.EncodeToString(buf.Bytes()) | ||
|
|
||
| mergeTxHexes := make([]string, len(mergeTxs)) | ||
| for i, tx := range mergeTxs { | ||
| var mergeBuf = new(bytes.Buffer) | ||
| tx.Serialize(mergeBuf) | ||
| mergeTxHexes[i] = hex.EncodeToString(mergeBuf.Bytes()) | ||
| } | ||
|
|
||
| updateRequest := UpdateTxRequest{ | ||
| Address: gc.Account.Address, | ||
| TxHex: txHex, | ||
| Values: values, | ||
| MergeTxs: mergeTxHexes, | ||
| } | ||
| jsonData, err := json.Marshal(updateRequest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| req, err := http.NewRequest("PUT", fmt.Sprintf("%s/transactions/%s", gc.URL, prevTxID.String()), strings.NewReader(string(jsonData))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := gc.Client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| errMsg, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return fmt.Errorf("failed to update transaction: %s", errMsg) | ||
| } | ||
|
|
||
| var updatedTxResp SignTxResponse | ||
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | ||
| } |
There was a problem hiding this comment.
Check the return values of updatedTx.Serialize(buf) and tx.Serialize(mergeBuf).
Static analysis flagged these calls (lines 225 & 231). Ignoring serialization errors could produce incomplete or invalid requests.
- updatedTx.Serialize(buf)
+ if err := updatedTx.Serialize(buf); err != nil {
+ return fmt.Errorf("failed to serialize updated transaction: %w", err)
+ }
...
for i, tx := range mergeTxs {
var mergeBuf = new(bytes.Buffer)
- tx.Serialize(mergeBuf)
+ err := tx.Serialize(mergeBuf)
+ if err != nil {
+ return fmt.Errorf("failed to serialize mergeTx: %w", err)
+ }
mergeTxHexes[i] = hex.EncodeToString(mergeBuf.Bytes())
}📝 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.
| func (gc *GuardianClient) updateTx(prevTxID chainhash.Hash, updatedTx *wire.MsgTx, mergeTxs []*wire.MsgTx, values []int64) error { | |
| var buf = new(bytes.Buffer) | |
| updatedTx.Serialize(buf) | |
| txHex := hex.EncodeToString(buf.Bytes()) | |
| mergeTxHexes := make([]string, len(mergeTxs)) | |
| for i, tx := range mergeTxs { | |
| var mergeBuf = new(bytes.Buffer) | |
| tx.Serialize(mergeBuf) | |
| mergeTxHexes[i] = hex.EncodeToString(mergeBuf.Bytes()) | |
| } | |
| updateRequest := UpdateTxRequest{ | |
| Address: gc.Account.Address, | |
| TxHex: txHex, | |
| Values: values, | |
| MergeTxs: mergeTxHexes, | |
| } | |
| jsonData, err := json.Marshal(updateRequest) | |
| if err != nil { | |
| return err | |
| } | |
| req, err := http.NewRequest("PUT", fmt.Sprintf("%s/transactions/%s", gc.URL, prevTxID.String()), strings.NewReader(string(jsonData))) | |
| if err != nil { | |
| return err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := gc.Client.Do(req) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| errMsg, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return err | |
| } | |
| return fmt.Errorf("failed to update transaction: %s", errMsg) | |
| } | |
| var updatedTxResp SignTxResponse | |
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | |
| if err != nil { | |
| return err | |
| } | |
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | |
| if err != nil { | |
| return err | |
| } | |
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | |
| } | |
| func (gc *GuardianClient) updateTx(prevTxID chainhash.Hash, updatedTx *wire.MsgTx, mergeTxs []*wire.MsgTx, values []int64) error { | |
| var buf = new(bytes.Buffer) | |
| if err := updatedTx.Serialize(buf); err != nil { | |
| return fmt.Errorf("failed to serialize updated transaction: %w", err) | |
| } | |
| txHex := hex.EncodeToString(buf.Bytes()) | |
| mergeTxHexes := make([]string, len(mergeTxs)) | |
| for i, tx := range mergeTxs { | |
| var mergeBuf = new(bytes.Buffer) | |
| if err := tx.Serialize(mergeBuf); err != nil { | |
| return fmt.Errorf("failed to serialize mergeTx: %w", err) | |
| } | |
| mergeTxHexes[i] = hex.EncodeToString(mergeBuf.Bytes()) | |
| } | |
| updateRequest := UpdateTxRequest{ | |
| Address: gc.Account.Address, | |
| TxHex: txHex, | |
| Values: values, | |
| MergeTxs: mergeTxHexes, | |
| } | |
| jsonData, err := json.Marshal(updateRequest) | |
| if err != nil { | |
| return err | |
| } | |
| req, err := http.NewRequest("PUT", fmt.Sprintf("%s/transactions/%s", gc.URL, prevTxID.String()), strings.NewReader(string(jsonData))) | |
| if err != nil { | |
| return err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| resp, err := gc.Client.Do(req) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| errMsg, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return err | |
| } | |
| return fmt.Errorf("failed to update transaction: %s", errMsg) | |
| } | |
| var updatedTxResp SignTxResponse | |
| err = json.NewDecoder(resp.Body).Decode(&updatedTxResp) | |
| if err != nil { | |
| return err | |
| } | |
| updatedTxBytes, err := hex.DecodeString(updatedTxResp.TxHex) | |
| if err != nil { | |
| return err | |
| } | |
| return updatedTx.Deserialize(strings.NewReader(string(updatedTxBytes))) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
225-225: Error return value of updatedTx.Serialize is not checked
(errcheck)
231-231: Error return value of tx.Serialize is not checked
(errcheck)
Summary by CodeRabbit