-
Notifications
You must be signed in to change notification settings - Fork 87
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
Contract Pipeline Scripts #561
Conversation
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/MovementDeployer.s.sol
Outdated
Show resolved
Hide resolved
MCR newMCRImplementation = new MCR(); | ||
timelock.schedule( | ||
address(deployment.mcrAdmin), | ||
0, |
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 just informs the hash of the operation. Right now, we would want these to collide to avoid redundant upgrades, right? Does that take care of everything? In what case would this take a nonce value?
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 actually schedules a transaction on the Timelock contract. It performs the call.
It's not feasible for us to include handling the execution of the timelock transaction imo. We'll be using the safe FE anyway, it takes way less time, it populates the transaction for us, handles signature for all the parties involved, etc.
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.
The delay should be the actual time-lock no? The value
is just used for the operation hash?
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.
safe FE
I don't recall agreeing on this. This supposed to be dispatched from CI.
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.
safe FE
I don't recall agreeing on this. This supposed to be dispatched from CI.
The 0 was for value in eth to be transactioned
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.
The 0 was for value in eth to be transactioned
Wdym?
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.
during deployment you have the option of passing a certain amount of eth
to subsequent calls because you might want to pay for something. This does not happen here.
protocol-units/settlement/mcr/contracts/script/helpers/deployments.json
Outdated
Show resolved
Hide resolved
Can you expand? |
This PR should be renamed something like "Contract Pipeline Deployment Scripts" for clarity. It doesn't include the actual automation, checks, etc., ofc. |
If I run back-to-back deployments on Holesky naively, I get new proxies. The intended usage needs to be documented.
|
For one shot, this appears to be correct, but we need some basic e2e checks. |
@0xPrimata what are the objectives for this PR? As we've discussed elsewhere, it's not supposed to be the complete pipeline nor even the final scripts. How do we want to determine success? |
I think that this is final once instead of scheduling a transaction, I call the safe API by proposing a transaction: https://docs.safe.global/sdk/api-kit#propose-a-transaction-to-the-service Believe this is also blocked by the MOVEToken merge. |
Yeah, so usage of Safe is underspecified. This pipeline work should last into next week at least. For the MOVEToken deployment--which is on a tighter timeline--I would go with whatever you're most comfortable with. |
9f6a054
to
8f52a3d
Compare
protocol-units/settlement/mcr/contracts/script/MultisigMOVETokenDeployer.s.sol
Show resolved
Hide resolved
protocol-units/settlement/mcr/contracts/script/StakingDeployer.s.sol
Outdated
Show resolved
Hide resolved
console.log("STAKING: deploying"); | ||
MovementStaking stakingImplementation = new MovementStaking(); | ||
vm.recordLogs(); | ||
stakingProxy = new TransparentUpgradeableProxy( |
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.
Why not UUPS?
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.
protocol-units/settlement/mcr/contracts/script/helpers/Create3/CREATE3Factory.sol
Show resolved
Hide resolved
@@ -0,0 +1,43 @@ | |||
// SPDX-License-Identifier: MIT | |||
pragma solidity ^0.8.19; | |||
|
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.
You got a link to what this was based on?
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.
🚢
protocol-units/settlement/mcr/contracts/src/token/faucet/MOVEFaucet.sol
Outdated
Show resolved
Hide resolved
Head up @0xPrimata @l-monninger #703 (error running local Suzuka + bridge after this was merged). I haven't investigated beyond trying to run Suzuka + bridge locally, will look deeper into it after getting L1 --> L2 E2E tests against framework done (working against commit right before 561 was merged, because local Suzuka + bridge works for that commit). |
Summary
scripts
.Handles contract deployment, upgrades and stores it using Foundry script.
Changelog
Testing
Relying on previous tests. Deployment is valid and stored values are valid.
If there are previous deployments, tests are run on top of existent deployments on specific environments.
Existent deployments are specified in
protocol-units/settlement/mcr/contracts/script/helpers/deployments.json
. If running on foundry localchain, all contracts will be deployed and upgraded.E.g. Simulate deployment on top of Sepolia existent contracts:
because safes and timelock exist on this environment, they will be skipped. All subsequent contracts should be deployed.
Outstanding issues