test: move btc revertAddress test to advanced group to avoid upgrade test failure #3205
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 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces updates to the changelog and several test files related to Bitcoin deposit functionalities. A new section titled "Unreleased" is added to the changelog, including a note about relocating a Bitcoin revert address test to an advanced group. Additionally, new test cases are introduced, and existing assertions in the Bitcoin deposit tests are modified to use stricter failure conditions, enhancing the robustness of the test suite without altering the overall control flow. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
ws4charlie
added
test
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (2)
Line range hint
11-11: Move test to advanced group per PR objectivesThis test should be moved to the advanced test group to prevent upgrade test failures as specified in the PR objectives. Consider adding a group identifier or moving the file to an advanced test directory.
Consider one of these approaches:
- Add a test group identifier:
-func TestBitcoinStdMemoDepositAndCallRevert(r *runner.E2ERunner, args []string) { +func TestBitcoinStdMemoDepositAndCallRevert_Advanced(r *runner.E2ERunner, args []string) {
- Move the file to an advanced test directory:
mkdir -p e2e/e2etests/advanced/ git mv e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go e2e/e2etests/advanced/
47-47: Improve logging format and informationThe current logging statement could be enhanced to provide more structured and detailed information about the test execution.
Consider updating the log message:
- r.Logger.Info("Sent %f BTC to TSS to call non-existing contract, got refund of %d satoshis", amount, value) + r.Logger.Info("Bitcoin deposit and call revert test completed", + "sent_btc", amount, + "refund_satoshis", value, + "receiver", receiver, + "test_group", "advanced")e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1)
42-43: LGTM! Consider enhancing error messages for better debugging.The change from
asserttorequireis a good improvement as it ensures immediate test failure on invalid conditions. However, we could make the error messages more descriptive.Consider adding more descriptive error messages:
-require.Equal(r, r.BTCDeployerAddress.EncodeAddress(), receiver) -require.True(r, value > 0) +require.Equal(r, r.BTCDeployerAddress.EncodeAddress(), receiver, "refund should be sent to deployer address") +require.True(r, value > 0, "refund amount should be positive")Additionally, consider validating that the refund amount matches the expected value (minus fees) for stronger verification.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (2)
Line range hint
1-47: Consider improving test robustness and explicitnessA few suggestions to enhance the test:
- Consider extracting the hardcoded revert address to a test constant or generating it dynamically
- Add explicit verification that the target contract doesn't exist before proceeding with the test
- Consider adding a comment explaining why sample.EthAddress() is used for a non-existing contract
Example improvement:
+ // Verify contract doesn't exist + exist := r.QueryContractExistence(sample.EthAddress()) + require.False(r, exist, "Contract should not exist")
48-54: LGTM! Consider enhancing value assertionsThe switch to
requireis appropriate as these are critical assertions. Two minor suggestions for improvement:
- Consider adding descriptive error messages to the require statements
- The value check could be more specific, verifying that the reverted amount is less than or equal to the original amount (accounting for fees)
Example improvements:
- require.Equal(r, cctx.InboundParams.Sender, r.BTCDeployerAddress.EncodeAddress()) + require.Equal(r, cctx.InboundParams.Sender, r.BTCDeployerAddress.EncodeAddress(), "Inbound sender should match deployer address") - require.True(r, value > 0) + require.True(r, value > 0 && value <= amount*1e8, "Reverted amount should be positive and not exceed original amount")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert.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)
✅ Files skipped from review due to trivial changes (1)
- changelog.md
🧰 Additional context used
📓 Path-based instructions (4)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
e2e/e2etests/test_bitcoin_deposit_and_call_revert.go (1)
Line range hint 1-47: Verify error handling for critical refund path.
This test verifies a critical path where user funds must be refunded correctly. We should ensure comprehensive error handling.
Let's verify the error handling implementation:
✅ Verification successful
Error handling for critical refund path is well-implemented.
The verification shows comprehensive error handling across the Bitcoin deposit and refund paths:
- The test verifies both the happy and error paths, including refund scenarios for invalid memos and dust amounts
- Error handling is implemented at multiple layers:
- Deposit fee calculation errors in
zetaclient/chains/bitcoin/observer/witness.go - Amount validation against minimum deposit fees
- Proper error propagation through the RPC layer
- End-to-end test coverage for refund scenarios with invalid memos and dust amounts
- Deposit fee calculation errors in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in Bitcoin deposit related code
# Look for error handling patterns in related files
# Check for error handling in Bitcoin deposit related files
rg -A 5 "Error|err|panic" --type go | grep -i "bitcoin.*deposit"
# Look for other revert test cases
ast-grep --pattern 'func Test.*Bitcoin.*Revert.*\(.*\) {
$$$
}'
Length of output: 16036
cmd/zetae2e/local/local.go (1)
341-341: LGTM! Test correctly moved to advanced group.
The test case has been appropriately placed in the bitcoinDepositTestsAdvanced slice, which aligns with the PR objective of moving it to prevent upgrade test failures.
Let's verify that the test constant exists in the e2etests package:
✅ Verification successful
✓ Test constant properly defined and referenced
The test constant TestBitcoinStdMemoDepositAndCallRevertOtherAddressName is correctly defined in e2e/e2etests/e2etests.go and appropriately referenced in the advanced test group.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test constant exists in e2etests package
rg -l "TestBitcoinStdMemoDepositAndCallRevertOtherAddressName.*=.*"
Length of output: 94
Script:
#!/bin/bash
# Let's verify the actual constant declaration to ensure it's properly defined
rg "TestBitcoinStdMemoDepositAndCallRevertOtherAddressName.*=.*" -A 1
Length of output: 340
…-replace-asset-with-require
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14567965 | Triggered | Generic Password | 69391aa | cmd/zetaclientd/start.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Description
Move
bitcoin_std_memo_deposit_and_call_revert_other_addressto advanced test group.Closes: #3204
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation