fix: use different database file names for btc signet and testnet4 #3225
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 several modifications across multiple components, primarily focusing on the Changes
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
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3225 +/- ##
===========================================
+ Coverage 62.00% 62.01% +0.01%
===========================================
Files 428 428
Lines 30469 30480 +11
===========================================
+ Hits 18891 18902 +11
Misses 10738 10738
Partials 840 840
|
![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 (1)
zetaclient/orchestrator/bootstrap.go (1)
481-493: Consider adding documentation for database file naming conventionThe implementation correctly handles database file naming across different Bitcoin networks. Consider adding a comment explaining the naming convention for future maintainers.
func btcDatabaseFileName(chain chains.Chain) string { + // btcDatabaseFileName returns the appropriate database file name for Bitcoin networks. + // For backward compatibility: + // - mainnet and testnet3 use the legacy name "btc_chain_client" + // - all other networks (e.g., signet, testnet4) use their chain name + // btcDatabaseFilename is the Bitcoin database file name now used in mainnet and testnet3 // so we keep using it here for backward compatibility const btcDatabaseFilename = "btc_chain_client"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 490-491: zetaclient/orchestrator/bootstrap.go#L490-L491
Added lines #L490 - L491 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
changelog.md(1 hunks)zetaclient/orchestrator/bootstrap.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
zetaclient/orchestrator/bootstrap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/orchestrator/bootstrap.go
[warning] 490-491: zetaclient/orchestrator/bootstrap.go#L490-L491
Added lines #L490 - L491 were not covered by tests
🔇 Additional comments (3)
zetaclient/orchestrator/bootstrap.go (2)
14-14: LGTM: Import addition is appropriate
The new import for the chains package is correctly placed and necessary for the new functionality.
363-363: LGTM: Database initialization updated correctly
The database initialization now uses the new btcDatabaseFileName function, ensuring proper separation of database files for different Bitcoin networks.
Let's verify the database file naming across different Bitcoin networks:
✅ Verification successful
Let me gather more information about the database file naming implementation.
Database file naming implementation verified successfully
The btcDatabaseFileName function implements a robust naming strategy:
- Maintains backward compatibility by using "btc_chain_client" for Bitcoin Mainnet and Testnet3
- Correctly handles additional Bitcoin networks by using their respective chain names
- Single point of control for database file naming, preventing naming conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check database file naming implementation across Bitcoin networks
# Look for any other instances where Bitcoin database files might be created
rg -A 2 "NewFromSqlite.*btc" --type go
Length of output: 343
Script:
#!/bin/bash
# Check the btcDatabaseFileName function implementation
ast-grep --pattern 'func btcDatabaseFileName($_) $_'
# Look for any other database file naming patterns for BTC
rg -B 2 -A 2 "database.*btc" --type go
Length of output: 1502
changelog.md (1)
17-17: LGTM - Changelog entry follows conventions
The new entry is properly formatted with:
- Correct categorization under "Fixes"
- Valid PR link (fix: use different database file names for btc signet and testnet4 #3225)
- Clear description of the change
| case chains.BitcoinMainnet.ChainId, chains.BitcoinTestnet.ChainId: | ||
| return legacyBTCDatabaseFilename | ||
| default: | ||
| return fmt.Sprintf("%s_%d", legacyBTCDatabaseFilename, chain.ChainId) |
There was a problem hiding this comment.
we use names of chains rather than chain ID currently so it would be good to keep that consistent?
zetachain@validator1-us-east-1-testnet:~/.zetaclient/chainobserver$ ls
amoy_testnet base_sepolia bsc_testnet btc_chain_client goerli_testnet mumbai_testnet scanner sepolia_testnet solana_devnet
Description
This PR picks necessary changes in the PR: #3224
All external observer signers must have already updated their zetaclient config files by next release, so there is no need to pick the special handling fallback logic around the old/new Bitcoin config.
How Has This Been Tested?
Summary by CodeRabbit