docs: improve deployment instructions #413
Conversation
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to the documentation and deployment scripts for Version 2 (V2) of the smart contract architecture. Key changes involve renaming sections in the README files for clarity, restructuring deployment instructions, and updating salt values in deployment scripts for 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 #413 +/- ##
=======================================
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: 1
🧹 Outside diff range and nitpick comments (6)
v2/README.md (1)
44-48: Consider adding a security note about private key handling.While the command structure is correct, it's recommended to add a security warning about not directly using private keys in command line arguments, as they may be logged in shell history.
Consider adding this note:
### Deploy ```shell $ forge script script/<DeployScript>.s.sol:<DeployScript> --rpc-url <your_rpc_url> --private-key <your_private_key>
+>
⚠️ Security Note: Avoid using private keys directly in command line arguments. Consider using environment variables or a.envfile instead.<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 47-47: null Dollar signs used before commands without showing output (MD014, commands-show-output) </blockquote></details> </details> </blockquote></details> <details> <summary>v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1)</summary><blockquote> `18-19`: **Consider documenting the versioning strategy.** The "-2" suffix in the salt values suggests a versioning strategy for deployments. This should be documented to maintain consistency across future updates. Consider adding a comment explaining the versioning strategy: ```diff + // Salt values include version suffix for deployment iteration tracking bytes32 implSalt = keccak256("GatewayEVM-2"); bytes32 proxySalt = keccak256("GatewayEVMProxy-2");v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1)
17-18: Document the new deterministic addresses.Since the salt values determine the final contract addresses, it would be helpful to document the expected addresses for different networks.
Consider adding a comment above these lines:
+ // Salt values for deterministic deployment v2 + // Expected addresses (mainnet): + // Implementation: 0x... + // Proxy: 0x... bytes32 implSalt = keccak256("ERC20Custody-2"); bytes32 proxySalt = keccak256("ERC20CustodyProxy-2");v2/scripts/deploy/readme.md (3)
1-13: Enhance readability with grammar fixes and markdown improvements.Please apply these refinements:
- Line 11: Rephrase to "check
.env.samplefor an example of how it should look"- Line 7: Add language specifier to the code block
-``` +```bash forge script scripts/deploy/<Script>.s.sol --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context. Context: ...pts, check `.env.sample` for example on how it should look like. Currently, `.env.sample` is set with t... (HOW_IT_SHOULD_BE) </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `60-80`: **Add context for blockscout verification and improve code block.** The section is well-documented, but consider: 1. Adding a brief explanation of why blockscout verification is used instead of etherscan 2. Adding language specifier to the code block ```diff -``` +```bash forge script scripts/deploy/deterministic/DeployGatewayZEVM.s.sol \ --private-key <PRIVATE_KEY> \ --rpc-url <RPC_URL> \ --verify \ --verifier blockscout \ --verifier-url <VERIFIER_URL> \ --chain-id <CHAIN_ID> \ --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [uncategorized] ~66-~66: Loose punctuation mark. Context: ...v` file: - `GATEWAY_ADMIN_ADDRESS_ZEVM`: address of the admin - `WZETA`: wrappe... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `82-97`: **Improve markdown formatting and grammar.** Please enhance this section with the following changes: 1. Format URLs as proper markdown links 2. Fix grammar by adding missing articles: - Line 85: "will be on the same address" - Line 87: "using the above technique" 3. Consider adding a note about the importance of salt values in create2 deployments ```diff -Deployment scripts in `deterministic` uses create2 with Foundry (https://book.getfoundry.sh/tutorials/create2-tutorial) +Deployment scripts in `deterministic` uses create2 with [Foundry](https://book.getfoundry.sh/tutorials/create2-tutorial) -The contract can be upgraded with the following documentation: https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades +The contract can be upgraded following the [OpenZeppelin Foundry Upgrades documentation](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades)🧰 Tools
🪛 LanguageTool
[uncategorized] ~85-~85: You might be missing the article “the” here.
Context: ... chainGatewayEVMcontract will be on same address. Since UUPS proxy is used for ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~87-~87: You might be missing the article “the” here.
Context: ...n andERC1967Proxyare deployed using above technique: - calculate expected addres...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
84-84: null
Bare URL used(MD034, no-bare-urls)
93-93: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- v2/README.md (1 hunks)
- v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/readme.md (0 hunks)
- v2/scripts/deploy/readme.md (1 hunks)
💤 Files with no reviewable changes (1)
- v2/scripts/deploy/deterministic/readme.md
🧰 Additional context used
🪛 Markdownlint
v2/README.md
50-50: Expected: h4; Actual: h5
Heading levels should only increment by one level at a time(MD001, heading-increment)
47-47: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
v2/scripts/deploy/readme.md
84-84: null
Bare URL used(MD034, no-bare-urls)
93-93: null
Bare URL used(MD034, no-bare-urls)
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
41-41: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
7-7: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
50-50: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
71-71: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🪛 LanguageTool
v2/scripts/deploy/readme.md
[grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context.
Context: ...pts, check.env.samplefor example on how it should look like. Currently,.env.sampleis set with t...(HOW_IT_SHOULD_BE)
[grammar] ~17-~17: There may an error in the verb form ‘be deploy’.
Context: ...ewayEVM and ERC20Custody contracts must be deploy to setup a new environment on a connect...(MD_BE_NON_VBP)
[grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...RC20Custody contracts must be deploy to setup a new environment on a connected chains...(NOUN_VERB_CONFUSION)
[grammar] ~17-~17: The plural noun “chains” cannot be used with the article “a”. Did you mean “a connected chain” or “connected chains”?
Context: ...be deploy to setup a new environment on a connected chains. GatewayEVM The following environ...(A_NNS)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...set in the.envfile: -TSS_ADDRESS: address of the TSS used on the network ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...et: -ERC20_CUSTODY_ADMIN_ADDRESS_EVM: address of the admin - `GATEWAY_PROXY_E...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~46-~46: You might be missing the article “the” here.
Context: ... proxy gateway contract deployed in previous step Once setup, the contract can be d...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~66-~66: Loose punctuation mark.
Context: ...vfile: -GATEWAY_ADMIN_ADDRESS_ZEVM: address of the admin -WZETA`: wrappe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~85-~85: You might be missing the article “the” here.
Context: ... chainGatewayEVMcontract will be on same address. Since UUPS proxy is used for ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~87-~87: You might be missing the article “the” here.
Context: ...n andERC1967Proxyare deployed using above technique: - calculate expected addres...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (5)
v2/README.md (1)
44-55: Documentation improvements look good!The changes effectively improve the deployment instructions by:
- Simplifying the main deployment section
- Adding clear references to detailed documentation
- Maintaining a good balance between concise overview and detailed guidance
The structure allows users to quickly find basic deployment commands while providing access to comprehensive instructions when needed.
🧰 Tools
🪛 Markdownlint
50-50: Expected: h4; Actual: h5
Heading levels should only increment by one level at a time(MD001, heading-increment)
47-47: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1)
18-19: Verify the impact of salt changes on deployment addresses.The salt modifications will generate new deterministic addresses for both the implementation and proxy contracts. This is important for deployment coordination and integration points.
Let's verify the existing deployments and compute the new addresses:
Consider:
- Document the new deterministic addresses in deployment documentation
- Plan for a migration strategy if existing contracts interact with the current deployment
- Update integration tests to use the new addresses
v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (2)
Line range hint
1-58: Implementation includes robust safety checks.The script follows deployment best practices with comprehensive validation:
- Pre-computes expected addresses
- Validates successful deployments
- Verifies address matches
- Confirms initialization parameters
17-18: Verify the impact of salt value changes on existing deployments.The salt values have been updated with a "-2" suffix, which will generate new deterministic addresses for both the implementation and proxy contracts. This change suggests a new deployment version.
Let's verify if these contracts are already deployed with the old salt values:
Consider documenting:
- The reason for changing the salt values
- The implications for existing deployments
- Migration steps if needed
✅ Verification successful
Let me gather more specific information about the ERC20Custody deployments and salt usage.
I'm looking for:
- Any existing deployment configurations
- Context around the deployment scripts
- Documentation mentioning the ERC20Custody deployments
This will help us understand if this is a new deployment or an upgrade to existing ones, and what impact the salt changes might have.
Based on the gathered information, I can now provide a final response:
Salt changes are part of a new deployment version and won't affect existing contracts
The changes to the salt values are intentional and documented in the deployment script. From the context:
- The deployment script is part of v2 contracts, while v1 has its own separate implementation
- The
-2suffix in the new salt values clearly indicates this is a new version deployment- The deployment documentation in
v2/scripts/deploy/readme.mdshows this is part of the standard deployment process for setting up new environmentsThe salt changes won't affect existing deployments as Create2 deployments are immutable - once deployed with a specific salt, they cannot be overwritten. The new salt values will simply deploy to different addresses, allowing for a clean v2 deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any references to the old salt values in the codebase # or existing deployments that might be affected by this change. # Test 1: Search for old salt values in deployment artifacts or addresses rg -l "ERC20Custody[^\-]" --type yaml --type json # Test 2: Search for any documentation or scripts referencing the old addresses rg -l "0x[a-fA-F0-9]{40}" --type md --type txtLength of output: 502
Script:
#!/bin/bash # Search for any existing deployment addresses or configurations related to ERC20Custody rg -l "ERC20Custody" --type-add 'config:*.{json,yaml,toml}' --type config # Look for deployment scripts or tests that might reference ERC20Custody rg "ERC20Custody" -A 5 -B 5 ./v2/scripts/deploy/ # Check for any existing deployment documentation fd -e md | xargs rg "ERC20Custody" -A 3 -B 3Length of output: 20525
v2/scripts/deploy/readme.md (1)
15-58: Fix grammar issues and enhance deployment instructions.Please apply these corrections:
- Line 17: Change "must be deploy" to "must be deployed"
- Line 17: Change "setup" to "set up" (verb form)
- Line 17: Fix "a connected chains" to "connected chains"
- Line 46: Add "the" before "previous step"
Additionally, consider adding validation notes for the environment variables to help users verify their values before deployment.
🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: There may an error in the verb form ‘be deploy’.
Context: ...ewayEVM and ERC20Custody contracts must be deploy to setup a new environment on a connect...(MD_BE_NON_VBP)
[grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...RC20Custody contracts must be deploy to setup a new environment on a connected chains...(NOUN_VERB_CONFUSION)
[grammar] ~17-~17: The plural noun “chains” cannot be used with the article “a”. Did you mean “a connected chain” or “connected chains”?
Context: ...be deploy to setup a new environment on a connected chains. GatewayEVM The following environ...(A_NNS)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...set in the.envfile: -TSS_ADDRESS: address of the TSS used on the network ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...et: -ERC20_CUSTODY_ADMIN_ADDRESS_EVM: address of the admin - `GATEWAY_PROXY_E...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~46-~46: You might be missing the article “the” here.
Context: ... proxy gateway contract deployed in previous step Once setup, the contract can be d...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
41-41: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
50-50: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
v2/README.md (1)
44-47: Enhance command documentation with placeholder descriptions.While the command structure is clear, consider adding descriptions for the placeholders to help users:
### Deploy ```shell -$ forge script script/<DeployScript>.s.sol:<DeployScript> --rpc-url <your_rpc_url> --private-key <your_private_key> +$ forge script script/<DeployScript>.s.sol:<DeployScript> --rpc-url <your_rpc_url> --private-key <your_private_key> + +# Where: +# <DeployScript>: Name of the deployment script (e.g., DeployERC20Custody) +# <your_rpc_url>: RPC endpoint URL for the target network +# <your_private_key>: Private key for the deployer account (without 0x prefix)<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 47-47: null Dollar signs used before commands without showing output (MD014, commands-show-output) </blockquote></details> </details> </blockquote></details> <details> <summary>v2/scripts/deploy/readme.md (4)</summary><blockquote> `5-9`: **Add language specification to the code block.** For better syntax highlighting and documentation consistency, specify the language for the code block. ```diff -``` +```bash forge script scripts/deploy/<Script>.s.sol --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `29-37`: **Add language specification to the deployment command block.** For better syntax highlighting and documentation consistency, specify the language for the code block. ```diff -``` +```bash forge script scripts/deploy/deterministic/DeployGatewayEVM.s.sol \ --private-key <PRIVATE_KEY> \ --rpc-url <RPC_URL> \ --verify \ --etherscan-api-key <ETHERSCAN_API_KEY> \ --chain-id <CHAIN_ID> \ --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `71-80`: **Add language specification and clarify verification details.** The code block needs language specification, and the verification parameters could use more explanation. ```diff -``` +```bash forge script scripts/deploy/deterministic/DeployGatewayZEVM.s.sol \ --private-key <PRIVATE_KEY> \ --rpc-url <RPC_URL> \ --verify \ --verifier blockscout \ --verifier-url <VERIFIER_URL> \ --chain-id <CHAIN_ID> \ --broadcastConsider adding a note explaining the expected format for `<VERIFIER_URL>` and why blockscout is used specifically for ZetaChain. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `1-107`: **Consider adding troubleshooting section and environment setup checklist.** The deployment instructions are well-structured, but could be enhanced with: 1. A troubleshooting section covering common deployment issues and their solutions 2. A pre-deployment checklist to ensure all prerequisites are met 3. Examples of expected output for successful deployments This would make the documentation more robust and user-friendly. Would you like me to help draft these additional sections? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context. Context: ...pts, check `.env.sample` for example on how it should look like. Currently, `.env.sample` is set with t... (HOW_IT_SHOULD_BE) --- [grammar] ~17-~17: There may an error in the verb form ‘be deploy’. Context: ...ewayEVM and ERC20Custody contracts must be deploy to setup a new environment on a connect... (MD_BE_NON_VBP) --- [grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space. Context: ...RC20Custody contracts must be deploy to setup a new environment on a connected chains... (NOUN_VERB_CONFUSION) --- [grammar] ~17-~17: The plural noun “chains” cannot be used with the article “a”. Did you mean “a connected chain” or “connected chains”? Context: ...be deploy to setup a new environment on a connected chains. **GatewayEVM** The following environ... (A_NNS) --- [uncategorized] ~23-~23: Loose punctuation mark. Context: ...set in the `.env` file: - `TSS_ADDRESS`: address of the TSS used on the network ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~45-~45: Loose punctuation mark. Context: ...et: - `ERC20_CUSTODY_ADMIN_ADDRESS_EVM`: address of the admin - `GATEWAY_PROXY_E... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~46-~46: You might be missing the article “the” here. Context: ... **proxy** gateway contract deployed in previous step Once setup, the contract can be d... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~66-~66: Loose punctuation mark. Context: ...v` file: - `GATEWAY_ADMIN_ADDRESS_ZEVM`: address of the admin - `WZETA`: wrappe... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~85-~85: Possible missing comma found. Context: ...ntracts. This ensures that on every EVM chain `GatewayEVM` contract will be on same a... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~85-~85: You might be missing the article “the” here. Context: ... chain `GatewayEVM` contract will be on same address. Since UUPS proxy is used for ... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~87-~87: You might be missing the article “the” here. Context: ...n and `ERC1967Proxy` are deployed using above technique: - calculate expected addres... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~101-~101: Loose punctuation mark. Context: ...`deterministic/DeployERC20Custody.s.sol`: deploy the ERC20 custody contract on a ... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> 84-84: null Bare URL used (MD034, no-bare-urls) --- 93-93: null Bare URL used (MD034, no-bare-urls) --- 19-19: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 41-41: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 50-50: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> </blockquote></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 eeb58c8af348ea29a3b7d9979b2cd6852c4ee67f and 3b31125ebc0e837435884924c47edaecd5acbd85. </details> <details> <summary>📒 Files selected for processing (2)</summary> * v2/README.md (1 hunks) * v2/scripts/deploy/readme.md (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>v2/README.md</summary><blockquote> 50-50: Expected: h4; Actual: h5 Heading levels should only increment by one level at a time (MD001, heading-increment) --- 47-47: null Dollar signs used before commands without showing output (MD014, commands-show-output) </blockquote></details> <details> <summary>v2/scripts/deploy/readme.md</summary><blockquote> 84-84: null Bare URL used (MD034, no-bare-urls) --- 93-93: null Bare URL used (MD034, no-bare-urls) --- 19-19: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 41-41: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 50-50: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> <details> <summary>🪛 LanguageTool</summary><blockquote> <details> <summary>v2/scripts/deploy/readme.md</summary><blockquote> [grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context. Context: ...pts, check `.env.sample` for example on how it should look like. Currently, `.env.sample` is set with t... (HOW_IT_SHOULD_BE) --- [grammar] ~17-~17: There may an error in the verb form ‘be deploy’. Context: ...ewayEVM and ERC20Custody contracts must be deploy to setup a new environment on a connect... (MD_BE_NON_VBP) --- [grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space. Context: ...RC20Custody contracts must be deploy to setup a new environment on a connected chains... (NOUN_VERB_CONFUSION) --- [grammar] ~17-~17: The plural noun “chains” cannot be used with the article “a”. Did you mean “a connected chain” or “connected chains”? Context: ...be deploy to setup a new environment on a connected chains. **GatewayEVM** The following environ... (A_NNS) --- [uncategorized] ~23-~23: Loose punctuation mark. Context: ...set in the `.env` file: - `TSS_ADDRESS`: address of the TSS used on the network ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~45-~45: Loose punctuation mark. Context: ...et: - `ERC20_CUSTODY_ADMIN_ADDRESS_EVM`: address of the admin - `GATEWAY_PROXY_E... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~46-~46: You might be missing the article “the” here. Context: ... **proxy** gateway contract deployed in previous step Once setup, the contract can be d... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~66-~66: Loose punctuation mark. Context: ...v` file: - `GATEWAY_ADMIN_ADDRESS_ZEVM`: address of the admin - `WZETA`: wrappe... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~85-~85: Possible missing comma found. Context: ...ntracts. This ensures that on every EVM chain `GatewayEVM` contract will be on same a... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~85-~85: You might be missing the article “the” here. Context: ... chain `GatewayEVM` contract will be on same address. Since UUPS proxy is used for ... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~87-~87: You might be missing the article “the” here. Context: ...n and `ERC1967Proxy` are deployed using above technique: - calculate expected addres... (AI_EN_LECTOR_MISSING_DETERMINER_THE) --- [uncategorized] ~101-~101: Loose punctuation mark. Context: ...`deterministic/DeployERC20Custody.s.sol`: deploy the ERC20 custody contract on a ... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>v2/README.md (1)</summary><blockquote> Line range hint `1-5`: **LGTM! Clear development status notice.** The notice effectively communicates the development status and provides a helpful link to get started with V2. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 50-50: Expected: h4; Actual: h5 Heading levels should only increment by one level at a time (MD001, heading-increment) --- 41-41: null Dollar signs used before commands without showing output (MD014, commands-show-output) --- 47-47: null Dollar signs used before commands without showing output (MD014, commands-show-output) </blockquote></details> </details> </blockquote></details> <details> <summary>v2/scripts/deploy/readme.md (1)</summary><blockquote> `99-107`: **LGTM! Clear and comprehensive list of deployment scripts.** The list provides a good overview of all available deployment scripts with clear descriptions of their purposes. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [uncategorized] ~101-~101: Loose punctuation mark. Context: ...`deterministic/DeployERC20Custody.s.sol`: deploy the ERC20 custody contract on a ... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> </details> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
v2/scripts/deploy/readme.md (7)
3-3: Fix grammar in introduction.The introduction could be more precise.
-This directory contains script to deploy the protocol contracts on ZetaChain and connected chains. +This directory contains scripts to deploy the protocol contracts on ZetaChain and connected chains.
7-9: Add language specifier to code block.For better syntax highlighting and documentation consistency, specify the language for the code block.
-``` +```shell forge script scripts/deploy/<Script>.s.sol --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `19-19`: **Use proper markdown headings.** Instead of using bold text for headers, use proper markdown heading syntax for better document structure. ```diff -**GatewayEVM** +### GatewayEVM -**ERC20Custody** +### ERC20CustodyAlso applies to: 41-41
🧰 Tools
🪛 Markdownlint
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
29-37: Add language specifier to code block.For better syntax highlighting and documentation consistency, specify the language for the code block.
-``` +```shell forge script scripts/deploy/deterministic/DeployGatewayEVM.s.sol \ --private-key <PRIVATE_KEY> \ --rpc-url <RPC_URL> \ --verify \ --etherscan-api-key <ETHERSCAN_API_KEY> \ --chain-id <CHAIN_ID> \ --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `71-80`: **Add language specifier to code block.** For better syntax highlighting and documentation consistency, specify the language for the code block. ```diff -``` +```shell forge script scripts/deploy/deterministic/DeployGatewayZEVM.s.sol \ --private-key <PRIVATE_KEY> \ --rpc-url <RPC_URL> \ --verify \ --verifier blockscout \ --verifier-url <VERIFIER_URL> \ --chain-id <CHAIN_ID> \ --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `82-97`: **Add security considerations for deterministic deployments.** While the deterministic deployment process is well explained, it would be helpful to add security considerations regarding the use of create2 and UUPS proxy pattern. Consider adding: - The importance of carefully managing the salt values - Security implications of proxy upgrades - Access control considerations for the upgrade process Would you like me to help draft a security considerations section? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 84-84: null Bare URL used (MD034, no-bare-urls) --- 93-93: null Bare URL used (MD034, no-bare-urls) </blockquote></details> </details> --- `101-107`: **Consider grouping scripts by functionality.** The script listing would be more organized if grouped by functionality: 1. Core Protocol Deployment - DeployERC20Custody.s.sol - DeployGatewayEVM.s.sol - DeployGatewayZEVM.s.sol 2. Upgrade Scripts - UpgradeGatewayEVM.s.sol 3. Test & Development - TestERC20.s.sol - DeployZRC20.s.sol - ZetaConnectorNonNative.s.sol Would you like me to help restructure this section? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [uncategorized] ~101-~101: Loose punctuation mark. Context: ...`deterministic/DeployERC20Custody.s.sol`: deploy the ERC20 custody contract on a ... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> </details> </blockquote></details> </blockquote></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 3b31125ebc0e837435884924c47edaecd5acbd85 and 2fbe3ae45ca6c077acfd09cf9837bfc3178c274c. </details> <details> <summary>📒 Files selected for processing (1)</summary> * v2/scripts/deploy/readme.md (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> <details> <summary>v2/scripts/deploy/readme.md</summary><blockquote> [grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context. Context: ...pts, check `.env.sample` for example on how it should look like. Currently, `.env.sample` is set with t... (HOW_IT_SHOULD_BE) --- [grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space. Context: ...20Custody contracts must be deployed to setup a new environment on a connected chain.... (NOUN_VERB_CONFUSION) --- [uncategorized] ~23-~23: Loose punctuation mark. Context: ...set in the `.env` file: - `TSS_ADDRESS`: address of the TSS used on the network ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~45-~45: Loose punctuation mark. Context: ...et: - `ERC20_CUSTODY_ADMIN_ADDRESS_EVM`: address of the admin - `GATEWAY_PROXY_E... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~66-~66: Loose punctuation mark. Context: ...v` file: - `GATEWAY_ADMIN_ADDRESS_ZEVM`: address of the admin - `WZETA`: wrappe... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~101-~101: Loose punctuation mark. Context: ...`deterministic/DeployERC20Custody.s.sol`: deploy the ERC20 custody contract on a ... (UNLIKELY_OPENING_PUNCTUATION) </blockquote></details> </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>v2/scripts/deploy/readme.md</summary><blockquote> 84-84: null Bare URL used (MD034, no-bare-urls) --- 93-93: null Bare URL used (MD034, no-bare-urls) --- 19-19: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 41-41: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 50-50: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 71-71: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: skosito <skostic9242@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
v2/scripts/deploy/readme.md (5)
5-9: Add language specification to code block.The code block should specify the language for better syntax highlighting.
-``` +```shell forge script scripts/deploy/<Script>.s.sol --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `29-37`: **Add language specification to deployment command blocks.** All deployment command blocks should specify the shell language for better syntax highlighting. ```diff -``` +```shellAlso applies to: 50-58, 71-80
🧰 Tools
🪛 Markdownlint
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
39-39: Clarify contract verification importance.While verification is technically optional for deployment, it's a crucial step for contract transparency and security. Consider adding a note about its importance.
-Note: `verify` and `etherscan-api-key` options are optional for the contract deployment itself. +Note: While `verify` and `etherscan-api-key` options are not required for deployment, contract verification is strongly recommended for transparency and security purposes.
101-107: Enhance script descriptions for clarity.Some script descriptions could be more detailed to better guide users.
-- `deterministic/UpgradeGatewayEVM.s.sol`: upgrade the GatewayEVM contract to a test contract implementation, used for test purposes only -- `deterministic/ZetaConnectorNonNative.s.sol`: deploy the ZETA connector contract on a connected chain, currently not used +- `deterministic/UpgradeGatewayEVM.s.sol`: [TEST ONLY] A script to test the upgrade mechanism of the GatewayEVM contract +- `deterministic/ZetaConnectorNonNative.s.sol`: [DEPRECATED] A script to deploy the ZETA connector contract (no longer used in production)🧰 Tools
🪛 LanguageTool
[uncategorized] ~101-~101: Loose punctuation mark.
Context: ...deterministic/DeployERC20Custody.s.sol: deploy the ERC20 custody contract on a ...(UNLIKELY_OPENING_PUNCTUATION)
19-19: Use proper markdown headings instead of bold text.For better document structure and navigation, use proper heading syntax.
-**GatewayEVM** +### GatewayEVM -**ERC20Custody** +### ERC20CustodyAlso applies to: 41-41
🧰 Tools
🪛 Markdownlint
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- v2/scripts/deploy/readme.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
v2/scripts/deploy/readme.md
[grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context.
Context: ...pts, check.env.samplefor example on how it should look like. Currently,.env.sampleis set with t...(HOW_IT_SHOULD_BE)
[grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...20Custody contracts must be deployed to setup a new environment on a connected chain....(NOUN_VERB_CONFUSION)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...set in the.envfile: -TSS_ADDRESS: address of the TSS used on the network ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...et: -ERC20_CUSTODY_ADMIN_ADDRESS_EVM: address of the admin - `GATEWAY_PROXY_E...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~46-~46: You might be missing the article “the” here.
Context: ... proxy gateway contract deployed in previous step Once setup, the contract can be d...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~66-~66: Loose punctuation mark.
Context: ...vfile: -GATEWAY_ADMIN_ADDRESS_ZEVM: address of the admin -WZETA`: wrappe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~87-~87: You might be missing the article “the” here.
Context: ...n andERC1967Proxyare deployed using above technique: - calculate expected addres...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~101-~101: Loose punctuation mark.
Context: ...deterministic/DeployERC20Custody.s.sol: deploy the ERC20 custody contract on a ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
v2/scripts/deploy/readme.md
84-84: null
Bare URL used(MD034, no-bare-urls)
93-93: null
Bare URL used(MD034, no-bare-urls)
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
41-41: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
7-7: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
50-50: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
71-71: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
v2/scripts/deploy/readme.md (7)
3-3: Fix typo in introduction.-This directory contains script to deploy the protocol contracts on ZetaChain and connected chains. +This directory contains scripts to deploy the protocol contracts on ZetaChain and connected chains.
7-9: Add language specification to code block.-``` +```shell forge script scripts/deploy/<Script>.s.sol --rpc-url <RPC_URL> --private-key <PRIVATE_KEY> --broadcast<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 7-7: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `17-17`: **Fix verb usage.** ```diff -The GatewayEVM and ERC20Custody contracts must be deployed to setup a new environment on a connected chain. +The GatewayEVM and ERC20Custody contracts must be deployed to set up a new environment on a connected chain.🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...20Custody contracts must be deployed to setup a new environment on a connected chain....(NOUN_VERB_CONFUSION)
19-19: Use proper markdown heading syntax.-**GatewayEVM** +### GatewayEVM -**ERC20Custody** +### ERC20CustodyAlso applies to: 46-46
🧰 Tools
🪛 Markdownlint
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
29-37: Add language specification to code blocks.Add
shelllanguage specification to the code blocks to enable proper syntax highlighting:-``` +```shellAlso applies to: 55-63
🧰 Tools
🪛 Markdownlint
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
76-85: Add language specification to code block.Add
shelllanguage specification to the code block:-``` +```shell🧰 Tools
🪛 Markdownlint
76-76: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
89-90: Improve documentation formatting and clarity.-Deployment scripts in `deterministic` uses create2 with Foundry (https://book.getfoundry.sh/tutorials/create2-tutorial) to perform deterministic deployment of contracts. +Deployment scripts in `deterministic` use create2 with Foundry ([Create2 Tutorial](https://book.getfoundry.sh/tutorials/create2-tutorial)) to perform deterministic deployment of contracts. -The contract can be upgraded with the following documentation: https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades. It requires: +The contract can be upgraded following the [OpenZeppelin Foundry Upgrades documentation](https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades). The upgrade process requires: -- deploy new implementation (doesn't need to be deterministic since proxy address doesn't change) -- use plugin to upgrade proxy +- Deploy a new implementation contract (doesn't need to be deterministic since the proxy address remains unchanged) +- Use the OpenZeppelin plugin to upgrade the proxy to point to the new implementationAlso applies to: 98-101
🧰 Tools
🪛 Markdownlint
89-89: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- v2/scripts/deploy/readme.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
v2/scripts/deploy/readme.md
[grammar] ~11-~11: ‘Like’ cannot be used with the question word ‘how’ in this context.
Context: ...pts, check.env.samplefor example on how it should look like. Currently,.env.sampleis set with t...(HOW_IT_SHOULD_BE)
[grammar] ~17-~17: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...20Custody contracts must be deployed to setup a new environment on a connected chain....(NOUN_VERB_CONFUSION)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...set in the.envfile: -TSS_ADDRESS: address of the TSS used on the network ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...et: -ERC20_CUSTODY_ADMIN_ADDRESS_EVM: address of the admin - `GATEWAY_PROXY_E...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~71-~71: Loose punctuation mark.
Context: ...vfile: -GATEWAY_ADMIN_ADDRESS_ZEVM: address of the admin -WZETA`: wrappe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...deterministic/DeployERC20Custody.s.sol: deploy the ERC20 custody contract on a ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
v2/scripts/deploy/readme.md
89-89: null
Bare URL used(MD034, no-bare-urls)
98-98: null
Bare URL used(MD034, no-bare-urls)
19-19: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
46-46: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
7-7: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
29-29: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
40-40: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
55-55: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
76-76: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Add more instructions in the readme specifically for the protocol contracts deployment
Summary by CodeRabbit
New Features
Bug Fixes
Documentation