docs: add scripts and section to help with ZRC20 deployment for ZRC20 upgrade #418
Conversation
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes introduce new utility scripts and a detailed deployment process for a ZRC20 reference contract in the Changes
Possibly related PRs
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 8 8
Lines 388 388
Branches 123 123
=======================================
Hits 327 327
Misses 61 61 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
v2/scripts/utils/get_zrc20_info.sh (1)
1-10: Enhance documentation with examples and error scenarios.
The documentation is clear but could be more helpful by including:
- Example command with actual values
- Sample output format
- Common error scenarios and troubleshooting steps
Add these sections to the documentation:
# Usage:
# ./get_zrc20_info.sh <contract_address> <rpc_url>
+#
+# Example:
+# ./get_zrc20_info.sh 0x1234...5678 https://rpc.example.com
+#
+# Output Format:
+# SYSTEM_CONTRACT=0x...
+# ZRC20_NAME=Token Name
+# ZRC20_SYMBOL=TKN
+# ...
+#
+# Common Errors:
+# - "Error: Contract not found": Verify contract address and RPC URL
+# - "Method not found": Contract might not be a valid ZRC20v2/scripts/utils/compare_zrc20_info.sh (1)
1-8: Consider enhancing the script documentation.
While the current documentation is clear, consider adding:
- Example output to show the expected format
- Description of exit codes
- License header to match the project's open-source nature
v2/scripts/deploy/readme.md (2)
87-93: LGTM! Consider adding a note about proxy patterns.
The introduction clearly explains the upgrade process. To enhance clarity, consider adding a brief explanation of why this contract doesn't follow the ERC1967Proxy standard, as this might help developers better understand the architectural decision.
96-98: Add language specifiers to code blocks.
For better syntax highlighting and documentation consistency, add language specifiers to the code blocks:
-```
+```shell
./scripts/utils/get_zrc20_info.sh <zrc20_address> <rpc>- +properties
SYSTEM_CONTRACT=0xEdf...795B
...
Also applies to: 104-112
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 Markdownlint</summary>
96-96: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
</details>
</details>
<details>
<summary>📜 Review details</summary>
**Configuration used: .coderabbit.yaml**
**Review profile: CHILL**
<details>
<summary>📥 Commits</summary>
Files that changed from the base of the PR and between 577403818960651383532c665ac1dba92d41f397 and a931f5a83f572f6113678b5ab9c6781de3d734ca.
</details>
<details>
<summary>📒 Files selected for processing (3)</summary>
* v2/scripts/deploy/readme.md (2 hunks)
* v2/scripts/utils/compare_zrc20_info.sh (1 hunks)
* v2/scripts/utils/get_zrc20_info.sh (1 hunks)
</details>
<details>
<summary>🧰 Additional context used</summary>
<details>
<summary>🪛 LanguageTool</summary>
<details>
<summary>v2/scripts/deploy/readme.md</summary>
[grammar] ~126-~126: Did you mean “verifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...nt, the following utility script allows to verify the ZRC20 has been deployed with the co...
(ALLOW_TO)
</details>
</details>
<details>
<summary>🪛 Markdownlint</summary>
<details>
<summary>v2/scripts/deploy/readme.md</summary>
96-96: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
104-104: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
115-115: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
127-127: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
</details>
<details>
<summary>🔇 Additional comments (5)</summary>
<details>
<summary>v2/scripts/utils/get_zrc20_info.sh (2)</summary>
`24-26`: **LGTM!**
Variable assignments are clear and well-named.
---
`28-35`: _:warning: Potential issue_
**Add error handling for RPC calls and improve robustness.**
The current implementation has several potential issues:
1. No error handling for failed RPC calls
2. No validation of contract responses
3. Sequential RPC calls might be slow
Implement error handling and response validation:
```diff
+# Function to make RPC calls with error handling
+make_rpc_call() {
+ local result
+ if ! result=$(cast call "$CONTRACT_ADDRESS" "$1" --rpc-url "$RPC_URL" 2>&1); then
+ echo "Error calling $1: $result" >&2
+ exit 1
+ fi
+ echo "$result"
+}
+
# Run the cast commands and capture the outputs
-SYSTEM_CONTRACT=$(cast call "$CONTRACT_ADDRESS" "SYSTEM_CONTRACT_ADDRESS()(address)" --rpc-url "$RPC_URL" | tr -d '\n')
+SYSTEM_CONTRACT=$(make_rpc_call "SYSTEM_CONTRACT_ADDRESS()(address)" | tr -d '\n')
+
+# Validate system contract address format
+if ! [[ $SYSTEM_CONTRACT =~ ^0x[a-fA-F0-9]{40}$ ]]; then
+ echo "Error: Invalid system contract address returned: $SYSTEM_CONTRACT"
+ exit 1
+fi
+
# Continue with other calls using make_rpc_call...
Let's verify the contract interface:
v2/scripts/utils/compare_zrc20_info.sh (1)
53-53: Verify completeness of ZRC20 attributes.
Please confirm that the ATTRIBUTES array includes all necessary fields for validating a ZRC20 upgrade. Consider checking the ZRC20 specification or existing contracts to ensure no critical attributes are missing.
✅ Verification successful
All necessary ZRC20 attributes are being verified
The ATTRIBUTES array in compare_zrc20_info.sh correctly includes all the essential attributes from the ZRC20 contract implementation:
SYSTEM_CONTRACT_ADDRESS- System contract referencename,symbol,decimals- Standard token metadataCHAIN_ID- Chain identifierCOIN_TYPE- Token type identifierGAS_LIMIT- Gas limit for operations
The script's attribute list matches all the key immutable and configurable properties defined in the ZRC20 contract that need to be compared during upgrades.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ZRC20 interface definitions and implementations
rg -A 10 "contract.*ZRC20.*{"
Length of output: 131
Script:
#!/bin/bash
# Search for ZRC20 interface or contract definitions
rg -l "ZRC20"
# Search for attribute definitions or constants in ZRC20-related files
rg -A 5 "(constant|attribute|field|property)" $(rg -l "ZRC20")
Length of output: 66664
Script:
#!/bin/bash
# Search for ZRC20 interface and implementation to find all attributes
rg -A 10 "interface.*ZRC20|contract.*ZRC20" v2/contracts/zevm/interfaces/IZRC20.sol v2/contracts/zevm/ZRC20.sol
# Search for attribute usage in get_zrc20_info.sh
cat v2/scripts/utils/get_zrc20_info.sh
Length of output: 7256
v2/scripts/deploy/readme.md (2)
156-156: LGTM! Clear description of dual purpose.
The updated description clearly communicates that the script serves both testing and upgrade processes.
102-112: Consider security implications of example values.
The example shows real contract addresses. Consider:
- Using placeholder addresses (e.g., 0x123...abc) in documentation
- Adding a warning about keeping contract addresses and configuration secure
Let's verify if these addresses are sensitive:
🧰 Tools
🪛 Markdownlint
104-104: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Add instruction for ZRC20 upgrade
Summary by CodeRabbit
get_zrc20_info.shfor retrieving ZRC20 contract attributes.compare_zrc20_info.shfor comparing attributes between two ZRC20 contracts.