refactor(e2e): sign bitcoin transactions locally#3753
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request refactors Bitcoin handling across multiple components by replacing deployer-specific methods with generalized ones. The changes update address retrieval to return keypairs via Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Function
participant E as E2ERunner
participant B as Bitcoin RPC Client
T->>E: GetBtcKeypair()
E-->>T: (Address, PrivateKey)
T->>E: ListUTXOs()
E-->>T: UTXO List
T->>E: SendToTSSFromWithMemo(amount, UTXOs, memo)
E->>B: Construct & Sign Transaction
B-->>E: Transaction Hash
E-->>T: Transaction Hash
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
Documentation and Community
|
gartnera
force-pushed
the
bitcoin-localsign
branch
from
263603a to
6f7a2ae
Compare
gartnera
force-pushed
the
bitcoin-localsign
branch
2 times, most recently
from
7aabf11 to
38151a5
Compare
gartnera
force-pushed
the
bitcoin-localsign
branch
from
38151a5 to
76f3f56
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go (1)
44-45: Consider refactoring repetitive test cases.The test structure shows three very similar test cases with only the memo parameter changing. Consider refactoring to a table-driven test approach to improve maintainability and reduce code duplication.
func TestBitcoinDepositInvalidMemoRevert(r *runner.E2ERunner, args []string) { require.Len(r, args, 0) // mine blocks at normal speed stop := r.MineBlocksIfLocalBitcoin() defer stop() + testCases := []struct { + name string + memo []byte + }{ + { + name: "deposit without memo", + memo: nil, + }, + { + name: "deposit empty memo", + memo: []byte{}, + }, + { + name: "deposit invalid memo", + memo: []byte("invalid memo"), + }, + } + + for _, tc := range testCases { + utxos := r.ListUTXOs() + txHash, err := r.SendToTSSFromWithMemo(0.1, utxos[:1], tc.memo) + require.NoError(r, err) + + // wait for the cctx to be mined + cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) + r.Logger.CCTX(*cctx, tc.name) + utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Reverted) + require.EqualValues(r, crosschaintypes.InboundStatus_INVALID_MEMO, cctx.InboundParams.Status) + } - // CASE 1 - // make a deposit without memo output - utxos := r.ListUTXOs() - txHash, err := r.SendToTSSFromWithMemo(0.1, utxos[:1], nil) - require.NoError(r, err) - - // wait for the cctx to be mined - cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) - r.Logger.CCTX(*cctx, "deposit without memo") - utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Reverted) - require.EqualValues(r, crosschaintypes.InboundStatus_INVALID_MEMO, cctx.InboundParams.Status) - - // CASE 2 - // make a deposit with a empty memo - utxos = r.ListUTXOs() - txHash, err = r.SendToTSSFromWithMemo(0.1, utxos[:1], []byte{}) - require.NoError(r, err) - - // wait for the cctx to be mined - cctx = utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) - r.Logger.CCTX(*cctx, "deposit empty memo") - utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Reverted) - require.EqualValues(r, crosschaintypes.InboundStatus_INVALID_MEMO, cctx.InboundParams.Status) - - // CASE 3 - // make a deposit with an invalid memo - utxos = r.ListUTXOs() - txHash, err = r.SendToTSSFromWithMemo(0.1, utxos[:1], []byte("invalid memo")) - require.NoError(r, err) - - // wait for the cctx to be mined - cctx = utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) - r.Logger.CCTX(*cctx, "deposit invalid memo") - utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Reverted) - require.EqualValues(r, crosschaintypes.InboundStatus_INVALID_MEMO, cctx.InboundParams.Status) }e2e/runner/setup_bitcoin.go (1)
69-73: Incorrect function comment for GetBtcAddress.The function comment states "GetBtcKeypair returns..." but should be "GetBtcAddress returns..." to accurately reflect the function's purpose.
-// GetBtcKeypair returns the BTC address of the runner account +// GetBtcAddress returns the BTC address of the runner account func (r *E2ERunner) GetBtcAddress() *btcutil.AddressWitnessPubKeyHash { address, _ := r.GetBtcKeypair() return address }e2e/runner/balances.go (1)
171-171: Consider handling unused private key more explicitly.The function now retrieves both the address and private key using
GetBtcKeypair()but discards the private key with_. While this is a common pattern, explicitly commenting on the intentional discard of the private key would improve code clarity, especially given the security implications.-address, _ := r.GetBtcKeypair() +// We only need the address for balance checking, not the private key +address, _ := r.GetBtcKeypair()e2e/runner/bitcoin.go (3)
206-248: Refactored transaction creation for local signing.The transaction creation logic has been completely rewritten to create and sign transactions locally instead of using the node's wallet RPCs. This is a key implementation of the PR's objective to eliminate the need for loading private keys into the Bitcoin node.
However, there are a few aspects that could be improved:
- There's no explicit fee calculation based on transaction size
- No handling for different script types (assuming all inputs are P2WPKH)
- No verification that inputs haven't been spent elsewhere
These issues might be acceptable in a test environment but should be considered for production use.
Consider adding more robust fee calculation based on transaction size and weight, and adding support for different script types.
249-251: Consider clarifying the purpose of rearranging outputs.The code swaps TxOut elements to move the memo output to the second position, but it's not immediately clear why this is necessary or what convention it's following.
// Move memo output to second position +// Bitcoin convention is to have recipient output first, memo second, and change last tx.TxOut[1], tx.TxOut[2] = tx.TxOut[2], tx.TxOut[1]
260-284: Implemented local transaction signing.The transaction signing process now happens locally instead of relying on the Bitcoin node's wallet. This implements the core objective of the PR to sign Bitcoin transactions locally.
The code correctly constructs the witness data for P2WPKH inputs using the
WitnessSignaturefunction, but assumes all inputs are of the same type.For more robust transaction signing, consider adding support for different script types by checking the script format before choosing the signing method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cmd/zetae2e/bitcoin_address.go(1 hunks)cmd/zetae2e/local/bitcoin.go(3 hunks)cmd/zetae2e/run.go(0 hunks)e2e/e2etests/e2etests.go(1 hunks)e2e/e2etests/helpers.go(2 hunks)e2e/e2etests/test_bitcoin_deposit.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_call.go(3 hunks)e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go(3 hunks)e2e/e2etests/test_bitcoin_donation.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go(1 hunks)e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go(1 hunks)e2e/e2etests/test_bitcoin_withdraw_segwit.go(1 hunks)e2e/e2etests/test_crosschain_swap.go(4 hunks)e2e/e2etests/test_stress_btc_withdraw.go(1 hunks)e2e/runner/balances.go(2 hunks)e2e/runner/bitcoin.go(13 hunks)e2e/runner/runner.go(0 hunks)e2e/runner/setup_bitcoin.go(5 hunks)zetaclient/chains/bitcoin/client/commands.go(0 hunks)zetaclient/chains/bitcoin/client/mockgen.go(0 hunks)
💤 Files with no reviewable changes (4)
- cmd/zetae2e/run.go
- e2e/runner/runner.go
- zetaclient/chains/bitcoin/client/commands.go
- zetaclient/chains/bitcoin/client/mockgen.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_withdraw_segwit.goe2e/e2etests/test_bitcoin_std_deposit_and_call.goe2e/e2etests/test_bitcoin_std_deposit_and_call_revert.goe2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.gocmd/zetae2e/bitcoin_address.gocmd/zetae2e/local/bitcoin.goe2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.goe2e/e2etests/e2etests.goe2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.goe2e/e2etests/test_stress_btc_withdraw.goe2e/e2etests/test_bitcoin_donation.goe2e/e2etests/helpers.goe2e/e2etests/test_bitcoin_deposit_call.goe2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.goe2e/e2etests/test_bitcoin_deposit_and_call_revert.goe2e/runner/balances.goe2e/e2etests/test_bitcoin_deposit.goe2e/runner/setup_bitcoin.goe2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.goe2e/e2etests/test_crosschain_swap.goe2e/runner/bitcoin.go
🧠 Learnings (2)
cmd/zetae2e/bitcoin_address.go (1)
Learnt from: gartnera
PR: zeta-chain/node#3105
File: e2e/runner/setup_bitcoin.go:51-69
Timestamp: 2025-03-26T19:42:34.108Z
Learning: In test code (`e2e/runner/setup_bitcoin.go`), adding security measures for private key handling in the `GetBtcAddress` method is not required.
e2e/runner/setup_bitcoin.go (1)
Learnt from: gartnera
PR: zeta-chain/node#3105
File: e2e/runner/setup_bitcoin.go:51-69
Timestamp: 2025-03-26T19:42:34.108Z
Learning: In test code (`e2e/runner/setup_bitcoin.go`), adding security measures for private key handling in the `GetBtcAddress` method is not required.
🧬 Code Definitions (5)
e2e/e2etests/test_bitcoin_std_deposit_and_call.go (1)
zetaclient/chains/bitcoin/common/tx_script.go (1)
EncodeAddress(238-242)
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
testutil/sample/crosschain.go (1)
InboundParams(176-191)
cmd/zetae2e/local/bitcoin.go (1)
e2e/e2etests/e2etests.go (1)
TestBitcoinDepositName(105-105)
e2e/e2etests/test_bitcoin_deposit_call.go (1)
zetaclient/chains/bitcoin/common/tx_script.go (1)
EncodeAddress(238-242)
e2e/runner/bitcoin.go (1)
e2e/runner/runner.go (1)
E2ERunner(80-204)
🔇 Additional comments (40)
e2e/e2etests/test_bitcoin_deposit_invalid_memo_revert.go (2)
20-21: Method signature change aligns with PR objectives.The change from deployer-specific UTXO listing (
ListDeployerUTXOs) to a more generalized approach (ListUTXOs) aligns with the PR's goal of removing deployer account references from test routines. This modification enhances test isolation and reduces potential race conditions as mentioned in the PR objectives.
32-33: Consistent refactoring approach.The method signature changes to
ListUTXOs()andSendToTSSFromWithMemo()maintain consistency with earlier changes in the file. This systematic approach to refactoring supports the PR's goal of enabling local Bitcoin transaction signing.e2e/e2etests/helpers.go (1)
31-41: Excellent addition of balance validation to prevent unclear errors.The new balance check ensures that withdrawals only proceed when sufficient funds are available, preventing cryptic "execution reverted" errors. This is a thoughtful improvement that aligns with the PR objectives and enhances the robustness of the test suite.
e2e/runner/setup_bitcoin.go (4)
24-27: Good refactoring to eliminate deployer-specific dependencies.The change to use
r.GetBtcKeypair()instead ofr.BTCDeployerAddressaligns with the PR objectives to remove references to the deployer account and make each test routine operate independently.
48-67: Well-structured keypair retrieval function.The implementation of
GetBtcKeypair()is clean and follows good practices for Bitcoin address derivation from private keys.
90-90: Significant change in wallet creation properties.Changing
disable_private_keysfromfalsetotrueis a significant modification that aligns with the PR objective of signing Bitcoin transactions locally rather than in the node. Ensure this has been thoroughly tested.
109-112: Good addition of address import for proper UTXO tracking.Adding the address import ensures proper indexing of UTXOs and transactions, which is essential for subsequent transaction operations.
e2e/e2etests/test_stress_btc_withdraw.go (1)
38-38: Appropriate refactoring to use the new address retrieval method.This change correctly replaces the direct use of deployer address with the new generalized method, which aligns with the PR's objective of removing references to the deployer account.
e2e/e2etests/test_bitcoin_withdraw_segwit.go (1)
15-15: Consistent refactoring of address retrieval.The change maintains consistency with the overall approach of eliminating deployer-specific references across the codebase.
e2e/e2etests/test_bitcoin_std_deposit_and_call.go (1)
53-53: Method reference updated to use generalized GetBtcAddressThe change from using
BTCDeployerAddress.EncodeAddress()toGetBtcAddress().EncodeAddress()is appropriate and aligns with the PR objective of removing deployer-specific references to prevent race conditions.cmd/zetae2e/bitcoin_address.go (1)
65-65: Method renamed to better reflect return valuesThe rename from
GetBtcAddress()toGetBtcKeypair()is semantically accurate as the method returns both an address and a private key. This change supports the PR objective of enabling local transaction signing.Let's verify that this method is used consistently across the codebase:
#!/bin/bash # Check if there are any remaining references to the old GetBtcAddress method rg "GetBtcAddress\(\)" --type goe2e/e2etests/e2etests.go (1)
825-825:Details
✅ Verification successful
Increased default Bitcoin deposit amount
The default Bitcoin amount for the deposit test has been increased from 0.001 BTC to 1.0 BTC, which is a 1000x increase. This supports the PR objective of using the bitcoin_deposit test to fund withdraw tests.
Before approving this change, verify that this substantial amount increase is necessary and won't consume excessive resources in the test environment:
🏁 Script executed:
#!/bin/bash # Check how the deposit amount is used in withdrawal tests rg "TestBitcoinDepositName" --type go -A 5 -B 5Length of output: 3014
Verified: Increased Bitcoin deposit amount is appropriate for funding withdrawal tests
The updated default value of 1.0 BTC for the
TestBitcoinDepositNametest has been confirmed as necessary. Our investigation shows that:
- The deposit test with a default value of 1.0 BTC is explicitly used to fund subsequent withdrawal tests in
cmd/zetae2e/local/bitcoin.go.- This change aligns with the PR's objective of ensuring sufficient funds for withdrawals.
- There is no evidence that this increased amount will cause excessive resource usage in the test environment.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
44-44: Updated address reference for revert transaction verificationThe test now correctly uses
GetBtcAddress().EncodeAddress()instead ofBTCDeployerAddress.EncodeAddress()when verifying the receiver of revert transactions. This change is consistent with the PR objective of removing deployer-specific references.e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
50-50: Appropriate transition from deployer-specific to generalized address retrieval.The change from
r.BTCDeployerAddress.EncodeAddress()tor.GetBtcAddress().EncodeAddress()aligns with the PR objective to remove deployer-specific references. This promotes test routine independence and helps prevent race conditions by retrieving the Bitcoin address dynamically instead of relying on a static deployer address.e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go (1)
43-43: Method renaming improves code generalization.The method update from
InscribeToTSSFromDeployerWithMemotoInscribeToTSSWithMemoeffectively removes the deployer-specific context, making the code more flexible and aligning with the PR's goal of eliminating deployer references. The parameters and functionality remain unchanged, preserving the test's behavior.cmd/zetae2e/local/bitcoin.go (4)
48-50: Good addition of bitcoin_deposit test as a dependency for withdraw tests.Adding the
e2etests.TestBitcoinDepositNameto the withdraw tests ensures proper funding for withdrawals, consistent with the PR objective to use the bitcoin_deposit test to fund withdraw tests. This mirrors the approach taken on the Ethereum side as mentioned in the PR description.
118-119: Refactored reward generation to use deployer runner.The change to use
deployerRunner.GetBtcAddress()instead of a hardcoded address for generating Bitcoin rewards eliminates deployer-specific references and makes the code more flexible. This supports the local transaction signing approach described in the PR objectives.
122-123: Simplified donation process.Replacing the combined method
DonateBTCAndDepositToTSS()with the more focusedDonateBTC()streamlines the initialization process and aligns with the PR's goal of making test routines more independent.
154-162: Well-structured initialization for local transaction signing.This block properly implements the local transaction signing approach by retrieving a Bitcoin keypair, ensuring the address is imported into the RPC client, and generating rewards to that address. The error handling is appropriate, and the sequence of operations is logical for establishing a funded address for testing.
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (2)
31-31: Generalized UTXO listing method.Replacing
r.ListDeployerUTXOs()withr.ListUTXOs()removes the deployer-specific reference, making the test more flexible and independent. This aligns with the PR objective to remove all references to the deployer account from each test routine.
40-41: Transaction sending method updated for consistency.The change from
r.SendToTSSFromDeployerWithMemotor.SendToTSSFromWithMemogeneralizes the transaction sending function, removing the deployer context. This supports the PR's goal of locally signing Bitcoin transactions and eliminating deployer dependencies.e2e/e2etests/test_bitcoin_donation.go (2)
22-22: Improved UTXO retrieval with generalized method.The change from
ListDeployerUTXOs()toListUTXOs()removes the dependency on a deployer-specific account, which aligns with the objective of making test routines operate independently to prevent race conditions.
27-27: Generalized transaction sending method implementation.The change from
SendToTSSFromDeployerWithMemo()toSendToTSSFromWithMemo()correctly eliminates the deployer-specific context in the transaction sending process, creating a more flexible and reusable approach.e2e/e2etests/test_bitcoin_deposit.go (3)
16-18: Clear documentation and accurate Bitcoin amount handling.The added comment and calculation for ZRC20 BTC amounts with 8 decimals improves code clarity and provides essential context for understanding Bitcoin amount handling.
19-20: Proper balance validation implementation.Retrieving the starting balance before the operation establishes a baseline for post-transaction validation, which is a necessary practice for comprehensive testing.
29-36:Details
✅ Verification successful
Comprehensive transaction validation with precise assertions.
The added assertions verify that:
- The inbound amount matches the expected value
- The balance increases by the expected amount after the deposit
This ensures the deposit functionality works correctly with appropriate tolerance checks.
🏁 Script executed:
#!/bin/bash # Verify the bigSub function used for balance difference calculation exists rg "func bigSub" --type goLength of output: 99
Validated Deposit Test Assertions
The assertions ine2e/e2etests/test_bitcoin_deposit.gocorrectly verify that the inbound deposit amount matches the expected value and that the account balance increases as anticipated. Additionally, the verification confirms that the supportingbigSubfunction exists (located ine2e/e2etests/helpers.go), which ensures that balance difference calculations are performed accurately. The implementation meets the necessary production-grade standards.e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (2)
35-35: Improved UTXO retrieval with generalized method.The change from
ListDeployerUTXOs()toListUTXOs()properly removes the deployer-specific context, which aligns with the objective of making test routines operate independently.
39-39: Generalized transaction sending method implementation.The change from
SendToTSSFromDeployerWithMemo()toSendToTSSFromWithMemo()appropriately eliminates the deployer-specific context, creating a more flexible transaction sending approach that can work with any address.e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (3)
23-23: Improved UTXO retrieval with generalized method.The change from
ListDeployerUTXOs()toListUTXOs()properly removes the deployer-specific dependency, making the test more isolated and less prone to race conditions.
30-30: Generalized transaction sending method implementation.The change from
SendToTSSFromDeployerWithMemo()toSendToTSSFromWithMemo()correctly eliminates the deployer-specific context in the transaction sending process, creating a more flexible approach.
40-40: Improved address retrieval with generalized method.The change from directly referencing
r.BTCDeployerAddress.EncodeAddress()to using the method callr.GetBtcAddress().EncodeAddress()provides a more encapsulated approach to address handling, which aligns with the objective of removing deployer-specific references and enabling local Bitcoin transaction signing.e2e/e2etests/test_bitcoin_deposit_call.go (3)
22-22: Modified UTXO retrieval to use generalized method.The function now uses
ListUTXOs()instead of the previous deployer-specific method, aligning with the PR's goal of eliminating deployer-specific functions to prevent race conditions in tests.
33-33: Updated transaction sending to use generalized method.This change removes the deployer-specific context from the transaction sending process, which is consistent with the PR objective of ensuring each test routine operates independently.
48-48: Updated Bitcoin address retrieval method.The code now uses
GetBtcAddress()instead of directly accessing the deployer address, providing more flexibility and aligning with the local signing approach.e2e/runner/balances.go (1)
189-189:Details
❓ Verification inconclusive
Removed spendability check from balance calculation.
The function now adds all unspent outputs to the total balance, regardless of their spendability status. This simplifies the logic but could potentially lead to reporting balances that aren't actually available for spending.
Verify that removing the spendability check doesn't impact test reliability by potentially reporting misleading balances. If this change was intentional as part of the refactoring, consider adding a comment explaining why spendability is no longer checked.
🏁 Script executed:
#!/bin/bash # Check for any other usages of spendability in UTXO handling rg --type go "Spendable" -A 3 -B 3Length of output: 21664
Action: Confirm intentional removal of the spendability check in e2e balance calculations.
It appears that the change in
e2e/runner/balances.go(line 189) removes the conditional check for spendability, so that all unspent outputs are summed regardless of whether they are actually available for spending. Our search shows that elsewhere in the codebase—especially in the simulation logic—the spendability is explicitly assessed via calls to methods likeSpendableCoins. This suggests that the removal is intentional for the e2e runner, likely to simplify reporting.Recommendations:
- Clarification: If this behavior is deliberate, please add an inline comment explaining why the spendability check has been removed in this context. This will help avoid confusion, as the simulation code explicitly checks spendability.
- Consistency Check: Ensure that the reported balance (which includes all unspent outputs) is acceptable for the intended e2e use and that the difference from simulation behavior is clearly documented or justified.
- Documentation: Update any relevant tests or external documentation to reflect that the e2e balance computation does not filter out non-spendable outputs.
e2e/e2etests/test_crosschain_swap.go (3)
76-76: Updated Bitcoin address encoding retrieval.Similar to changes in other files, this modification replaces the direct access to the deployer address with a more flexible approach using
GetBtcAddress().
101-101: Replaced deployer-specific UTXO listing with generalized method.These changes use the new
ListUTXOs()method instead ofListDeployerUTXOs(), which aligns with the PR objective of removing all references to the deployer account from each test routine.Also applies to: 141-141
112-112: Updated transaction sending to use generalized method.The code now uses
SendToTSSFromWithMemo()instead of the deployer-specific method, maintaining consistency with changes throughout the codebase.Also applies to: 142-143
e2e/runner/bitcoin.go (2)
35-38: Renamed and updated UTXO listing method.The function has been renamed from
ListDeployerUTXOstoListUTXOsand now retrieves the Bitcoin address dynamically usingGetBtcKeypair(), enhancing flexibility and aligning with the PR's goal of local transaction signing.
53-53:Details
❓ Verification inconclusive
Simplified UTXO filtering logic.
The conditional check for spendability has been removed, with filtering now only based on the amount being greater than or equal to 1.0 BTC. This simplification could potentially affect the selection of UTXOs for tests.
Verify that removing the spendability check doesn't impact test reliability. If this was intentional, consider adding a comment explaining the rationale.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that might rely on spendability filtering rg -A 3 -B 3 "Spendable" "e2e/e2etests/*.go"Length of output: 102
Review Assurance: UTXO Filtering Simplification Evaluation
The change now filters UTXOs solely based on an amount ≥ 1.0 BTC, completely removing the previous spendability check. Our initial attempt to search for tests referencing “Spendable” under
e2e/e2etests/*.goreturned an error (directory not found). To further confirm that no tests depend on the removed spendability logic, please run additional verifications—such as a broader repository search (e.g.,rg -i "spendab" .)—or manually inspect any relevant tests.If, after manual review, you determine that the removal is intentional, please add an inline comment documenting the rationale behind simplifying the UTXO filtering. This will help maintain clarity for future reviewers and maintainers.
3 participants
Sign bitcoin transactions locally rather than making an RPC call. This removes the need to load private keys into the bitcoin node.
Also:
bitcoin_deposittest to fund the withdraw test just like we do on the ethereum sideexecution revertederrors. Related to Return raw revert code if not unpackable ethermint#234We probably should be a bit more intelligent about the input UTXOs but that does not seem to be necessary right now.
Summary by CodeRabbit
Refactor
Tests
Chores