feat(scratch deploy): mvp of local scratch deployment #790
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| contract WithdrawalsManagerStub { | ||
| /** | ||
| * @dev Receives Ether. | ||
| * | ||
| * Currently this is intentionally not supported since Ethereum 2.0 withdrawals specification | ||
| * might change before withdrawals are enabled. This contract sits behind a proxy that can be | ||
| * upgraded to a new implementation contract collectively by LDO holders by performing a vote. | ||
| * | ||
| * When Ethereum 2.0 withdrawals specification is finalized, Lido DAO will prepare the new | ||
| * implementation contract and initiate a vote among LDO holders for upgrading the proxy to | ||
| * the new implementation. | ||
| */ | ||
| receive() external payable { | ||
| revert("not supported"); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Contracts that lock Ether
| function proxy_upgradeTo(address newImplementation, bytes memory setupCalldata) external { | ||
| address admin = _getAdmin(); | ||
| require(admin != address(0), "proxy: ossified"); | ||
| require(msg.sender == admin, "proxy: unauthorized"); | ||
|
|
||
| _upgradeTo(newImplementation); | ||
|
|
||
| if (setupCalldata.length > 0) { | ||
| Address.functionDelegateCall(newImplementation, setupCalldata, "proxy: setup failed"); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return
| constructor(address _logic, bytes memory _data) payable { | ||
| assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)); | ||
| _setImplementation(_logic); | ||
| if(_data.length > 0) { | ||
| Address.functionDelegateCall(_logic, _data); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return
deployed-goerli.json
Outdated
There was a problem hiding this comment.
the json files are quite different for different networks, for example this is how the accounting oracle section looks like for goerli and holesky:
"accountingOracle": {
"parameters": {
"deployParameters": {
"consensusVersion": 1
},
"contract": "AccountingOracle",
"implementation": "0xC94ad2eb129146235f71cc35f7FbFec7fe404DF9",
"address": "0x76f358A842defa0E179a8970767CFf668Fc134d6",
"constructorArgs": [
"0x1eDf09b5023DC86737b59dE68a8130De878984f5",
"0x1643E812aE58766192Cf7D2Cf9567dF2C37e9B7F",
"0x24d8451BC07e7aF4Ba94F69aCDD9ad3c6579D9FB",
12,
1616508000
]
},"accountingOracle": {
"deployParameters": {
"consensusVersion": 1
},
"proxy": {
"contract": "contracts/0.8.9/proxy/OssifiableProxy.sol",
"address": "0x4E97A3972ce8511D87F334dA17a2C332542a5246",
"constructorArgs": [
"0x6AcA050709469F1f98d8f40f68b1C83B533cd2b2",
"0x22896Bfc68814BFD855b1a167255eE497006e730",
"0x"
]
},
"implementation": {
"contract": "contracts/0.8.9/oracle/AccountingOracle.sol",
"address": "0x6AcA050709469F1f98d8f40f68b1C83B533cd2b2",
"constructorArgs": [
"0x28FAB2059C713A7F9D8c86Db49f9bb0e96Af1ef8",
"0x3F1c547b21f65e10480dE3ad8E19fAAC46C95034",
"0x072f72BE3AcFE2c52715829F2CD9061A6C8fF019",
12,
1695902400
]
}
},May we bring them to the same format and make a uniform order (maybe sort the keys alphabetically)?
There was a problem hiding this comment.
I'm pro for the unified order and for the alphabetical one: seems to be the most seamless among other options.
There was a problem hiding this comment.
I support the suggestion by @avsetsin 👍
scripts/helpers/deploy.js
Outdated
| address: contract.address, | ||
| constructorArgs: constructorArgs, | ||
| } | ||
| persistNetworkState2(network.name, netId, state) |
There was a problem hiding this comment.
persistNetworkState2 -> persistNetworkState ?
There was a problem hiding this comment.
removed duplicate function, left the more straightforward version of it
scripts/helpers/deploy.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| const { readNetworkState, persistNetworkState2 } = require('./persisted-network-state') | |||
| const { readNetworkState, persistNetworkState, persistNetworkState2 } = require('./persisted-network-state') | |||
There was a problem hiding this comment.
do we need 2 funcs like persistNetworkState ?
or maybe rename persist Network State 2 to understand the difference with persistNetworkState ?
There was a problem hiding this comment.
actually, we don't )
persistNetworkState2 was introduced by me once upon a time to maintain compatibility with all other existing scripts, but I also agree that it is a bit redundant
There was a problem hiding this comment.
removed duplicate function, left the more straightforward version of it
deployed-goerli.json
Outdated
There was a problem hiding this comment.
I support the suggestion by @avsetsin 👍
|
|
||
| if (implementation === null) { | ||
| console.log(`Deploying implementation for proxy of ${artifactName}`) | ||
| process.stdout.write(`Deploying implementation for proxy of ${artifactName}... `) |
There was a problem hiding this comment.
why sometimes console.log but here process.stdout?
There was a problem hiding this comment.
process.stdout.write is for printing without end of line (just to save some space on the screen)
|
@arwer13 could you please explicitly secure the quorum-sufficient TESTLDO amount to reside on the Agent's balance during the deployment That would ease governance-related test fixtures greatly. @bulbozaur proposes the idea, and I'd suggest implementing it |
The other comments are processed as well. |
arwer13
dismissed
TheDZhon’s stale review
TheDZhon is OoO, but he approved beforehand in messanger.
Renovation of the scratch deploy scripts. For the README see docs/scratch-deploy.md
NB that the scripts are to undertake one more refactoring iteration during migration to TypeScript and new repo https://github.com/lidofinance/core