-
Notifications
You must be signed in to change notification settings - Fork 91
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
build: add mainnet, testnet, dev migrations #842
Conversation
Hi @rndquu is this still a draft or review can start? |
Hey, the PR is draft |
Hi @rndquu I started the review, checked out clean pull request, built from scratch and executed deployment on Anvil testnet. Step 1 cp
I used the command from readme
Therefore, the 'smoke test' passed. I noticed that the deployment folders in
with my current content:
I will now go through the commits and changes. |
packages/contracts/migrations/development/Deploy001_Diamond_Dollar.s.sol
Show resolved
Hide resolved
I agree. Would still be useful to see the full list of skipped facets in a commit message, stating why the given facet may not be needed. Perhaps also such information would help auditors. |
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 went through all changes and in general I have minor findings. After those will be addressed / answered I will approve from my side.
packages/contracts/migrations/mainnet/Deploy002_Governance.s.sol
Outdated
Show resolved
Hide resolved
packages/contracts/migrations/mainnet/Deploy002_Governance.s.sol
Outdated
Show resolved
Hide resolved
packages/contracts/migrations/development/Deploy001_Diamond_Dollar.s.sol
Outdated
Show resolved
Hide resolved
From my point of view
|
all right, makes sense |
I'm performing last testing before pass @rndquu |
I'm deploying to actual testnet (not forked) while it does deploy the contracts after another run, it fails on the first run: but then the script re-run compilation: https://sepolia.etherscan.io/tx/0x603a56a6aa1ff3b59f80516e264f8431ab79a4aae9df3a69d2ac7cbbc4511637 I have matched bytecode and the above can be find at run-latest.json It's the script failing expected?? @rndquu more debug needed it? |
Did you check the failing tx on etherscan? There must be some clue of why it failed |
please check the screenshots, the revert it's when the script it's running, not when the contracts are getting deployed, so no etherscan, but yeah, would need to debug, also this happen continuously, when you run the script |
There is failing tx on |
that revert it's not getting shipped, it's locally happening but not getting shipped you can check the account https://sepolia.etherscan.io/address/0x940c5d8a6ac44ba0583162d32b05c079950b1ef3 the 2 transactions are the after the second re-run, it fails on the first run, then the script goes on, and deploys |
@molecula451 Do both owner and admin accounts have enough ether? |
yes, more than enough I'd say, 5 sepoliaETH, and it only cost less than them those 2 deployments |
can you reproduce it? @rndquu, indeed doesn't seem to happen on anvil fork |
the revert it's happening from here https://github.com/ubiquity/ubiquity-dollar/pull/842/files#diff-4c325704f2117271b3d06b56268d88bd646de602e36926d84e28f2dba7d8968cR276 |
yes, that's because LUSD is not actually deployed on sepolia testnet, I'll fix the error, thank you |
Notice that I moved the collateral token address to LUSD token (used as collateral in UbiquityPoolFacet) is not deployed to sepolia testnet so for testnet we should use some other token. I've checked the migration to testnet with DAI as collateral token and the migration executed fine. |
so the bug was LUSD was not deployed |
yes |
for your info you do not have to run all these commands, that's why we are updating the readme #843 check yarn deploy:development just do yarn.mp4 |
aaaaaaaaaaaaaaaaaaaaaaaaaand!! @rndquu Successfully set Dollar |
rndqnuu just updated the contracts
|
packages/contracts/migrations/development/Deploy001_Diamond_Dollar.s.sol
Show resolved
Hide resolved
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.
Basically all the debug happened on this PR. This PR is more than ready to be merge
I'm merging this one. Please leave your Review @gitcoindev |
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.
All right! I went through all the changes and comments again and approving. Good job @rndquu . We will also test this pr in future development.
what a great PR and great review and feedback happened here, congrats everyone, here this appealing, and is demanding excellent |
Resolves #827
This PR adds and modifies 2 commands:
yarn deploy:mainnet
: deploys the Diamond and Dollar contracts to ethereum mainetyarn deploy:development
: deploys Diamond, Dollar, UbiquityAlgorithmicDollarManager and UbiquityGovernance contracts to local anvil instance or testnet (depending on the RPC_URL env variable)There is no any difference between migrations to a local anvil instance and testnet so there is no separate command for testnet (i.e. there is no
yarn deploy:testnet
).The only difference between
yarn deploy:mainnet
andyarn deploy:development
is thatyarn deploy:mainnet
uses already deployed UbiquityAlgorithmicDollarManager and UbiquityGovernance contracts whileyarn deploy:development
deploys them from scratch.The deploy diamond migration deploys not all of the facets because chances are that we're not gonna need them + it's better to deploy exactly what we're gonna use without unnecessary "dead" contracts.
Other remarks:
P.S. "Audit & Fix" workflow is failing but it is not related to the current PR, already fixed here