feat: add signet and testnet4 addresses #435
Conversation
gartnera
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
skosito
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces two new entries to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
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
|
gartnera
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
+ Coverage 84.06% 86.37% +2.31%
==========================================
Files 8 8
Lines 389 499 +110
Branches 124 124
==========================================
+ Hits 327 431 +104
- Misses 62 68 +6 ☔ View full report in Codecov by Sentry. |
|
@gartnera ZRC-20 addresses are fetched automatically from https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/fungible/foreign_coins. Can you, please, move: {
"address": "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur",
"category": "omnichain",
"chain_id": 18333,
"chain_name": "btc_signet",
"type": "tss"
},
{
"address": "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur",
"category": "omnichain",
"chain_id": 18334,
"chain_name": "btc_testnet4",
"type": "tss"
},To |
So I/we can remove all the zrc20 entries?
I already have entries in the addresses.testnet.json file? |
|
@gartnera not exactly. This script: https://github.com/zeta-chain/protocol-contracts/blob/main/v2/tasks/addresses.ts And this file (containing addresses we can't fetch automatically): https://github.com/zeta-chain/protocol-contracts/blob/main/v2/tasks/addresses.testnet.json (note it's in the tasks dir) ❗️ I propose we add Bitcoin TSS addresses to this file and not the auto-generated file. Results in this auto-generated file: https://github.com/zeta-chain/protocol-contracts/blob/main/v2/data/addresses.testnet.json (note it's in the data dir) |
|
I know this system is crap (auto-generation breaking all the time), but the one we had before (manually maintained file, wrong addresses everywhere) was even worse. Let's do it right this time! 😃 https://www.notion.so/zetachain/Contract-Address-Registry-1517b3fb9049800e9eb4e292ca9867f0?pvs=4 |
gartnera
force-pushed
the
signet-testnet4
branch
from
57a4e7c to
399f3b1
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
🧹 Outside diff range and nitpick comments (1)
v1/tasks/addresses.testnet.json (1)
44-56: Consider adding documentation for manual entriesSince these addresses are being manually maintained (as discussed in the PR comments), consider adding a comment in the JSON to indicate this is intentional and to prevent future automated scripts from removing these entries.
Add a comment before these entries:
}, + // Manual entries for Bitcoin networks - DO NOT REMOVE + // These addresses are intentionally maintained here instead of being auto-generated { "address": "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
v1/data/addresses.testnet.jsonis excluded by!v1/data/**v2/data/addresses.testnet.jsonis excluded by!v2/data/**
📒 Files selected for processing (1)
v1/tasks/addresses.testnet.json(1 hunks)
🔇 Additional comments (2)
v1/tasks/addresses.testnet.json (2)
44-56: LGTM on JSON structure and format
The new entries maintain consistent formatting with the existing entries and include all required fields (address, category, chain_id, chain_name, type).
45-49: Verify the safety of using identical addresses across different Bitcoin networks
The same Bitcoin address tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur is being used for both signet and testnet4 networks. While technically possible, this practice could potentially lead to operational confusion or security risks.
Let's verify the Bitcoin address format and network compatibility:
Also applies to: 52-56
✅ Verification successful
Let me gather more context about the usage patterns of this address across different networks.
Using the same Bitcoin address across test networks is an established pattern
The address reuse across Bitcoin test networks (testnet, signet, testnet4) appears to be an intentional and established pattern in the codebase. This is evidenced by:
- The same pattern exists in both v1 and v2 configurations
- The address is consistently used as a TSS (Threshold Signature Scheme) endpoint
- All instances are properly categorized as "omnichain" and "type: tss"
- The address format is valid for Bitcoin testnet networks (validated by regex)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Bitcoin address format and search for any existing usage
# Expected results: Address should be a valid testnet/signet format (tb1 prefix)
# and we should find any other instances where this address is used
# Check if the address follows Bitcoin testnet bech32 format
echo "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur" | grep -E "^tb1[a-zA-HJ-NP-Z0-9]{39,59}$" || echo "Invalid testnet address format"
# Search for any other instances of this address in the codebase
rg "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur"
Length of output: 1094
Script:
#!/bin/bash
# Let's examine the complete entries in both v1 and v2 files to understand the network contexts
# where this address is being used
echo "=== v1/tasks/addresses.testnet.json ==="
cat v1/tasks/addresses.testnet.json
echo -e "\n=== v2/data/addresses.testnet.json ==="
cat v2/data/addresses.testnet.json
echo -e "\n=== v1/data/addresses.json ==="
cat v1/data/addresses.json
echo -e "\n=== v1/data/addresses.testnet.json ==="
cat v1/data/addresses.testnet.json
Length of output: 30664
|
Ok @fadeev I think I know what you're saying. I've updated the PR. If this still isn't correct maybe you could fix it up? |
gartnera
force-pushed
the
signet-testnet4
branch
from
399f3b1 to
8db4a3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/tasks/addresses.testnet.json (1)
77-77: Newtsstype introducedThe entries introduce a new
type: "tss"which differs from the existinggatewayandcustodytypes. This appears intentional for Bitcoin networks, but please ensure:
- The type is documented in the relevant specifications
- Any dependent systems are updated to handle this new type
Also applies to: 84-84
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
v1/data/addresses.testnet.jsonis excluded by!v1/data/**v2/data/addresses.testnet.jsonis excluded by!v2/data/**
📒 Files selected for processing (2)
v1/tasks/addresses.testnet.json(1 hunks)v2/tasks/addresses.testnet.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v1/tasks/addresses.testnet.json
🔇 Additional comments (3)
v2/tasks/addresses.testnet.json (3)
71-84: LGTM: JSON structure and formatting
The new entries maintain consistent structure and formatting with existing entries. The JSON is valid and properly formatted.
73-73: Verify Bitcoin address format and ownership
The Bitcoin testnet address format is valid (native SegWit with 'tb1' prefix). However, since this is a critical TSS address, please ensure:
- The address was generated using the correct network parameters
- The TSS key shares are properly distributed and backed up
Also applies to: 80-80
✅ Verification successful
Bitcoin testnet address is already in use across multiple configurations
The address tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur is already being used in both v1 and v2 configurations across multiple files:
v1/tasks/addresses.testnet.jsonv1/data/addresses.testnet.jsonv1/data/addresses.jsonv2/tasks/addresses.testnet.jsonv2/data/addresses.testnet.json
This confirms that:
- The address is a known and validated TSS address
- The key shares are already distributed and in use
- The address is being consistently used across different network configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if this address is already in use in testnet
# Note: This helps prevent address collisions and confirms ownership
# Search for any existing references to this address
rg "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur" --type json
Length of output: 1157
75-76: Verify chain IDs and network configurations
The chain IDs (18333 for signet, 18334 for testnet4) match Bitcoin's standard testnet identifiers. Please ensure:
- These chain IDs are consistently used across the entire protocol
- Network-specific configurations are properly set for each chain
Also applies to: 82-83
✅ Verification successful
Chain IDs and network configurations are consistent across the codebase
The verification shows that the chain IDs (18333 for btc_signet and 18334 for btc_testnet4) are consistently used across both v1 and v2 versions in the following files:
- v1/data/addresses.testnet.json
- v1/tasks/addresses.testnet.json
- v2/data/addresses.testnet.json
- v2/tasks/addresses.testnet.json
Additionally, there's a proper cross-chain reference in v2/data/addresses.testnet.json where "foreign_chain_id": "18333" is used to link with the Bitcoin signet network.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify chain ID consistency across the codebase
# Search for any existing references to these chain IDs
echo "Checking signet chain ID..."
rg "18333|btc_signet" --type json -A 5 -B 5
echo "Checking testnet4 chain ID..."
rg "18334|btc_testnet4" --type json -A 5 -B 5
Length of output: 7182
|
Removed testnet 4 and signet addresses from the manual JSON file, because they're fetched automatically. Updated the script to take chain names from |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
v2/tasks/addresses.ts (3)
Line range hint
32-73: Consider adding error handling for new Bitcoin networks.The fetchTssData function should handle potential errors specific to signet and testnet4 networks.
Consider adding network-specific error handling:
const fetchTssData = async (chains: any, addresses: any, network: Network) => { const bitcoinChainID = network === "zeta_mainnet" ? "8332" : "18332"; + // Map network-specific chain IDs + const bitcoinChainIDs = { + btc_testnet: "18332", + btc_signet_testnet: "18333", + btc_testnet4: "18334" + }; const URL = `${api[network].rpc}/zeta-chain/observer/get_tss_address/${bitcoinChainID}`; try { const tssResponse: AxiosResponse<any> = await axios.get(URL); if (tssResponse.status === 200) { chains.forEach((chain: any) => { const { btc, eth } = tssResponse.data; const isEVM = chain.consensus === "ethereum"; const isBitcoin = chain.consensus === "bitcoin"; + // Validate Bitcoin network type + if (isBitcoin && !bitcoinChainIDs[chain.name]) { + console.warn(`Unsupported Bitcoin network: ${chain.name}`); + return; + } if (isEVM || isBitcoin) { addresses.push({ address: isBitcoin ? btc : eth,
Line range hint
1-273: Consider improving address generation architecture.Given the issues with the generator mentioned in the PR objectives, consider these architectural improvements:
- Add type safety for chain name fields
- Implement a unified chain name handling strategy
- Add validation for supported networks
Would you like help designing a more robust address generation system that better handles different Bitcoin networks?
Use
chain.chain_nameinstead ofchain.namefor consistencyThe codebase consistently uses
chain.chain_nameacross both v1 and v2 versions, with only one instance usingchain.namein the modified code. Update the property access to maintain consistency:- chain_name: chain.name, + chain_name: chain.chain_name,🔗 Analysis chain
Line range hint
32-73: Ensure consistency with other functions using chain data.The change to use
chain.namein fetchTssData should be consistent across all functions handling chain information.Let's check for consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of chain_name vs name in chain handling echo "Files using chain_name:" rg "chain\.chain_name" --type ts echo -e "\nFiles using chain.name:" rg "chain\.name" --type tsLength of output: 1483
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
v2/data/addresses.testnet.jsonis excluded by!v2/data/**
📒 Files selected for processing (2)
v2/lib/types.ts(1 hunks)v2/tasks/addresses.ts(1 hunks)
🔇 Additional comments (2)
v2/lib/types.ts (1)
2-2: LGTM! Type changes align with new Bitcoin networks.
The addition of "btc_signet_testnet" and "btc_testnet4" to ParamChainName is appropriate. Removal of "empty" improves type accuracy.
Let's verify that removing "empty" doesn't break existing code:
✅ Verification successful
Removal of "empty" from ParamChainName is safe
The search results confirm that:
- There are no usages of "empty" as a chain name in the TypeScript codebase
- The ParamChainName type is consistently used across v1 and v2 directories
- The only places using ParamChainName are type declarations and the getAddress function, none of which reference an "empty" chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the removed "empty" chain name
rg -l '"empty"' --type ts
Length of output: 27
Script:
#!/bin/bash
# Let's try a more comprehensive search for "empty" in TypeScript files
# This will show the actual content and context where "empty" appears
rg "empty" --type ts -B 2 -A 2
# Also search for any type declarations or assignments using ParamChainName
rg "ParamChainName.*=|:.*ParamChainName" --type ts -B 2 -A 2
Length of output: 3763
v2/tasks/addresses.ts (1)
68-68: Verify API response structure for chain name field.
The change from chain.chain_name to chain.name suggests a change in API response structure. This could affect how Bitcoin signet and testnet4 addresses are mapped.
Let's verify the API response structure:
✅ Verification successful
Let me gather more information about how the chain fields are used across the codebase.
API response structure change is correct and consistent
The change from chain.chain_name to chain.name in fetchTssData is correct as both fields exist in the API response. The codebase shows a clear pattern:
chain.nameis used for TSS addresses in v2chain.chain_nameis used for other contract addresses (zetaToken, connector, erc20Custody)
This split usage is intentional as TSS addresses follow a different mapping structure compared to other contract addresses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch and examine the API response structure for chain fields
curl -s "https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/observer/supportedChains" | jq '.' > chain_response.json
echo "Chain fields in response:"
jq '.chains[0] | keys' chain_response.json
Length of output: 345
Script:
#!/bin/bash
# Search for usages of chain.name and chain.chain_name
echo "=== Searching for chain.name usage ==="
rg "chain\.name" -A 2
echo -e "\n=== Searching for chain.chain_name usage ==="
rg "chain\.chain_name" -A 2
echo -e "\n=== Checking the full context of addresses.ts ==="
cat v2/tasks/addresses.ts
Length of output: 14436
|
CI is failing because RPC endpoints are blocking requests. |
This is for v1 it doesn't matter anymore isn't it? @fadeev |
🤦♂️ I didn't notice it's only failing for v1. Yes, we can merge, and then disable generated CI for v1. |
|
In fact, shall I disable both generated and test for v1, so we have clean CI before merging? |
Yes |
The generator wasn't working so I added these by hand. The generator code also needs to be updated to correctly handle these new networks.
Related to zeta-chain/node#2242
Summary by CodeRabbit
New Features
btc_signetandbtc_testnet4.Improvements
Type Updates
Workflows