-
Notifications
You must be signed in to change notification settings - Fork 0
feat: prepare mainnet #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…or Ethereum and Arbitrum mainnets
| pragma solidity ^0.8.22; | ||
|
|
||
| import {Script, console} from "forge-std/Script.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No important change just cleaning
| pragma solidity ^0.8.22; | ||
|
|
||
| import {Script, console} from "forge-std/Script.sol"; | ||
| import {ConfigLib} from "./lib/ConfigLib.sol"; | ||
| import {IexecLayerZeroBridge} from "../src/bridges/layerZero/IexecLayerZeroBridge.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No important change just cleaning
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
=======================================
Coverage ? 83.78%
=======================================
Files ? 4
Lines ? 111
Branches ? 7
=======================================
Hits ? 93
Misses ? 17
Partials ? 1 ☔ View full report in Codecov by Sentry. |
.github/workflows/deploy.yml
Outdated
| env: | ||
| ETHERSCAN_API_KEY: ${{ secrets.ETHERSCAN_API_KEY }} | ||
| SEPOLIA_RPC_URL: ${{ vars.SEPOLIA_RPC_URL }} | ||
| ARBITRUM_SEPOLIA_RPC_URL: ${{ vars.ARBITRUM_SEPOLIA_RPC_URL }} | ||
| ETHEREUM_RPC_URL: ${{ secrets.RPC_URL }} | ||
| ARBITRUM_RPC_URL: ${{ secrets.RPC_URL }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be adapted to environment. secrets.RPC_URL will be sepolia not mainnet on certain Github envs.
We can use names like SOURCHAIN_CHAIN_RPC_URL and DESTINATION_CHAIN_RPC_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sry I don't get this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we call make verify-all-${{ inputs.network }}
we need only one of 4 RPC to be sets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use names like SOURCHAIN_CHAIN_RPC_URL and DESTINATION_CHAIN_RPC_URL
Since we run this script 1 network per 1 , I don't see how to use that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna check if I can put single RPC_URL on verification.mk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… in .env.template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR prepares the bridge system for mainnet deployment by adding support for Ethereum and Arbitrum mainnets alongside the existing testnet support. The changes include configuration updates, script modifications, verification targets, and documentation enhancements.
- Added mainnet configuration for Ethereum and Arbitrum networks with proper chain IDs and endpoint addresses
- Refactored verification system to use parameterized templates supporting both testnet and mainnet networks
- Updated cross-chain transfer scripts with improved error handling, logging, and mainnet-specific functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/verification.mk | Refactored verification system to use parameterized templates for both testnet and mainnet networks |
| script/SendFromEthereumToArbitrum.s.sol | Enhanced transfer script with better logging, error handling, and mainnet support |
| script/SendFromArbitrumToEthereum.s.sol | Enhanced transfer script with improved structure and mainnet compatibility |
| config/config.json | Added mainnet network configurations with proper addresses and chain IDs |
| README.md | Updated documentation to reflect mainnet support and usage instructions |
| Makefile | Added mainnet transfer targets and cleaned up deployment options |
| .github/workflows/deploy.yml | Implemented contract verification in CI workflow |
| .env.template | Reorganized environment template with mainnet RPC URLs and better structure |
Comments suppressed due to low confidence (2)
script/SendFromEthereumToArbitrum.s.sol:38
- Using 'RECIPIENT_ADDRESS' environment variable for the sender is confusing. Consider renaming to 'SENDER_ADDRESS' or using a more descriptive variable name.
address sender = vm.envAddress("RECIPIENT_ADDRESS");
script/SendFromArbitrumToEthereum.s.sol:42
- Using 'RECIPIENT_ADDRESS' environment variable for the sender is confusing. Consider renaming to 'SENDER_ADDRESS' or using a more descriptive variable name.
address sender = vm.envAddress("RECIPIENT_ADDRESS");
| "initialAdmin": "0x111165a109feca14e4ad4d805f6460c7d206ead1", | ||
| "initialUpgrader": "0x111121e2ec2557f484f65d5b1ad2b6b07b8acd23", | ||
| "initialPauser": "0x11113fe3513787f5a4f5f19690700e2736b3056e", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a TODO for that in the readme ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean
todo for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need two scripts? Can’t we have a single contract script that handles the transfer in both directions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could yes
will make a todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.