-
Notifications
You must be signed in to change notification settings - Fork 14
Add CreateX factory for new chain deployment #215
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
…kchainComputing/PoCo into feature/create-x-clean
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #215 +/- ##
========================================
Coverage 84.50% 84.50%
========================================
Files 35 35
Lines 1084 1084
Branches 221 221
========================================
Hits 916 916
Misses 168 168 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
config/config.json
Outdated
"usefactory": true, | ||
"Factory": "0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed", |
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.
"usefactory": true, | |
"Factory": "0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed", | |
"factory": "0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed", |
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.
Was to match with the other addresses but I can change to lower case :)
"AppRegistry": null,
"DatasetRegistry": null,
"WorkerpoolRegistry": null
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.
Lowercase is not really the issue, mentioning merge :)
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.
oh sorry, thanks for 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.
deploy/0_deploy.ts
Outdated
const rlcFactory = new RLC__factory().connect(owner); | ||
if (token) { | ||
console.log(`Using existing RLC token at: ${token}`); | ||
await deployments.save('RLC', { |
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.
I guess you are doing this for later deployments.get('RLC')
. For which network is it missing?
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.
Yes doing that for arbitrum sepolia actually and local test on hardhat those two deploy the RLC contract and the addresses are different
deploy/0_deploy.ts
Outdated
const rlcContract = await rlcFactory.deploy(); | ||
await rlcContract.waitForDeployment(); | ||
const rlcAddress = await rlcContract.getAddress(); |
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 rlcContract = await rlcFactory.deploy(); | |
await rlcContract.waitForDeployment(); | |
const rlcAddress = await rlcContract.getAddress(); | |
const rlcAddress = await rlcFactory.deploy() | |
.then((contract) => contract.waitForDeployment()) | |
.then((contract) => contract.getAddress()); |
?
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 isFujiFork = process.env.FUJI_FORK == 'true'; | ||
const isArbitrumSepoliaFork = process.env.ARBITRUM_SEPOLIA_FORK == 'true'; |
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.
Keeping only a single variable isFork
doesn't fit us?
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.
Here is to select the current chain to fork either local as bellecour, fuji, or arbi
Maybe I can re-work on it, but did not seen yet how I could change it
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 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.
Made some research, can't figure it out a better way than what we have rn
hardhat.config.ts
Outdated
etherscan: { | ||
apiKey: { | ||
mainnet: process.env.ETHERSCAN_API_KEY || '', | ||
fuji: process.env.SNOWTRACE_API_KEY || '', |
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.
Which purpose is it?
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.
Make it possible to verify poco in case of new deployment of these chains (?)
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.
oups typo I guess
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.
hardhat.config.ts
Outdated
bellecour: 'nothing', // a non-empty string is needed by the plugin. | ||
}, | ||
customChains: [ | ||
{ |
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.
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.
thanks
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.
Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com>
…ership transfer into separate functions
…d logic for factory address handling and deployment
…ove factory initialization
…Arbitrum Sepolia configurations
…pdate references in loadHardhatFixtureDeployment
…lization; update config for optional factoryType and salt
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.
Clean!
No description provided.