-
Notifications
You must be signed in to change notification settings - Fork 8
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
Omnichain governance deployments #57
Omnichain governance deployments #57
Conversation
helpers/deploy/deploymentUtils.ts
Outdated
import bscTestnetGovernanceDeployments from "../../deployments/bsctestnet.json"; | ||
import { SUPPORTED_NETWORKS } from "./constants"; | ||
|
||
export const testnetNetworks = ["sepolia", "opbnbtestnet", "arbitrumsepolia"]; |
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.
export const testnetNetworks = ["sepolia", "opbnbtestnet", "arbitrumsepolia"]; | |
export const testnetNetworks = ["sepolia", "opbnbtestnet", "arbitrumsepolia", "hardhat"]; |
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.
Scripts are working fine on hardhat, we don't need this change we need this only here
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.
helpers/deploy/deploymentUtils.ts
Outdated
import bscTestnetGovernanceDeployments from "../../deployments/bsctestnet.json"; | ||
import { SUPPORTED_NETWORKS } from "./constants"; | ||
|
||
export const testnetNetworks = ["sepolia", "opbnbtestnet", "arbitrumsepolia"]; | ||
const mainnetNetworks = ["ethereum", "opbnbmainnet", "arbitrumone"]; |
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.
const mainnetNetworks = ["ethereum", "opbnbmainnet", "arbitrumone"]; | |
const mainnetNetworks = ["ethereum", "opbnbmainnet", "arbitrumone", "hardhat"]; |
Is the quickest way to test this in development to deploy both sides of governance to hardhat? My thought is to start this way for testing/ subgraph testing and then iterate.
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.
Scripts are working fine on hardhat, we don't need this change.
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.
Correct the work fine but what we do loose without this change is deploying the target network contracts to hardhat which could be helpful for testing for example.
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 still deploy target network contracts along with local network contracts on hardhat .
For hardhat it will take bscmainnet chain id by default, see here
LMK if I am missing something.
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.
ca57fb8
to
3ddf06d
Compare
helpers/deploy/constants.ts
Outdated
hardhat: 10102, | ||
}; | ||
|
||
export const BNB_GUARDIAN = "0x1C2CAc6ec528c20800B2fe734820D87b581eAA6B"; |
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 move this variable to deploy/003-omnichain-source.ts
, where it's only used
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.
helpers/deploy/deploymentUtils.ts
Outdated
|
||
export const getSourceChainId = async (network: SUPPORTED_NETWORKS) => { | ||
if (testnetNetworks.includes(network as string)) { | ||
return 10102; |
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 use the constant LZ_CHAINID.bsctestnet
?
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.
Description
Resolves #