-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: csm el stealing penalty settling #71
Conversation
8bd164e
to
5670cc9
Compare
5670cc9
to
21ef7fa
Compare
c462162
to
054c5cb
Compare
chore: shanghai hardfork
* feat: deploy script * feat: acceptance deploy script
Co-authored-by: Eugene Mamin <TheDZhon@gmail.com>
return abi.decode(_evmScriptCallData, (uint256[])); | ||
} | ||
|
||
function _validateInputData( |
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 is no check on duplicates, wont it cause any issues? Haven't found this case covered covered by tests in CSM repo.
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.
+1
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.
added here lidofinance/community-staking-module#346
@@ -4,8 +4,7 @@ | |||
from utils import lido | |||
|
|||
|
|||
def test_grant_executor_permissions(accounts): | |||
lido_contracts = lido.contracts(network=brownie.network.show_active()) |
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.
unused lido imports in files after switching to fixture
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.
fixed
network-config.yaml
Outdated
chain_id: 17000 | ||
evm_version: shanghai | ||
fork: $HOLESKY_RPC_URL | ||
gas_limit: 12000000000 |
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.
Whats the reason for extra holesky network with such a huge gas limit?
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.
it was needed for devnet purposes. will remove
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.
removed
@@ -5,8 +5,7 @@ | |||
from utils import lido | |||
|
|||
|
|||
def test_revoke_permissions(accounts): | |||
lido_contracts = lido.contracts(network=network.show_active()) |
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.
Also import 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.
fixed
log.nb("All factories have been deployed.") | ||
log.nb("Saving atrifacts...") | ||
|
||
with open(f"deployed-{network_name}.json", "w") as outfile: |
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.
Seems like this will rewrite the current deployed-{network_name}.json
file, consider using different name for the file
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.
done
@@ -13,7 +13,7 @@ development: | |||
- cmd: ./ganache.sh | |||
cmd_settings: | |||
accounts: 10 | |||
evm_version: istanbul | |||
evm_version: shanghai |
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.
Do we really need a Shanghai version in tests? The compiler version will still be Istanbul, as contracts are written on the 0.8.6 Solidity version.
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 we need. because contract calls csm contract in _validateInput
when createEVMScript
return abi.decode(_evmScriptCallData, (uint256[])); | ||
} | ||
|
||
function _validateInputData( |
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.
+1
655d02b
to
7ab457b
Compare
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.
LGTM, but there are some addresses missing in the configurations that need to be filled in after the CSM deployment to pass the mainnet-fork tests before merge.
23bb90d
to
8ebf896
Compare
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.
LGTM
All commits were moved to #78 |
EasyTrack factory for CSM EL stealing penalty workflow