Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Gates contracts #377

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

Gates contracts #377

wants to merge 7 commits into from

Conversation

skhomuti
Copy link
Contributor

No description provided.

@skhomuti
feat: changeable early adoption
580c215 
Added a new early adoption implementation with changeable tree root and curve id

feat: add a possibility to change the early adoption address for the module. Now it's fully optional and doesn't set in the initializer

feat: added `curveExists` public method for CSBondCurve
@skhomuti
feat: changeable early adoption
25fecb1 
Added a new early adoption implementation with changeable tree root and curve id

feat: add a possibility to change the early adoption address for the module. Now it's fully optional and doesn't set in the initializer

feat: added `curveExists` public method for CSBondCurve
@skhomuti
temp commit
cfd8209
dgusakov
dgusakov requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@dgusakov dgusakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed partially without tests

script/DeployBase.s.sol Outdated
@@ -204,26 +206,26 @@ abstract contract DeployBase is Script {
accounting.MANAGE_BOND_CURVES_ROLE(),
address(deployer)
);
uint256 eaCurveId = accounting.addBondCurve(
config.earlyAdoptionBondCurve
uint256 vettedBondCurve = accounting.addBondCurve(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming to the identifiedSolosCurve

script/DeployImplementationsBase.s.sol Show resolved Hide resolved
script/DeployImplementationsBase.s.sol Outdated
@@ -84,12 +94,16 @@ abstract contract DeployImplementationsBase is DeployBase {
});

JsonObj memory deployJson = Json.newObj();
deployJson.set(
"PermissionlessGateImpl",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"PermissionlessGateImpl",
"PermissionlessGate",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a typo indeed

script/DeployImplementationsBase.s.sol Show resolved Hide resolved
script/DeployMainnet.s.sol
config
.earlyAdoptionTreeRoot = 0x359e02c5c065c682839661c9bdfaf38db472629bf5f7a7e8f0261b31dc9332c2; // See the first value in artifacts/mainnet/early-adoption/merkle-tree.json
config.earlyAdoptionBondCurve = new uint256[](2);
.vettedGateTreeRoot = 0x359e02c5c065c682839661c9bdfaf38db472629bf5f7a7e8f0261b31dc9332c2; // See the first value in artifacts/mainnet/early-adoption/merkle-tree.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the root before deployment to avoid double claming

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing to do right now in the code here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment pls

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is room for frontrunning anyway. if somebody decides to claim EA before CSM plug-in the gate for v2

src/CSModule.sol Outdated
function claimBeneficialBondCurve(
uint256 nodeOperatorId,
uint256 curveId
) external onlyRole(CLAIM_BENEFICIAL_CURVE_ROLE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) external onlyRole(CLAIM_BENEFICIAL_CURVE_ROLE) {
) external onlyRole(SET_BOND_CURVE_ROLE) {

src/CSModule.sol Outdated
Comment on lines 46 to 47
bytes32 public constant CLAIM_BENEFICIAL_CURVE_ROLE =
keccak256("CLAIM_BENEFICIAL_CURVE_ROLE");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bytes32 public constant CLAIM_BENEFICIAL_CURVE_ROLE =
keccak256("CLAIM_BENEFICIAL_CURVE_ROLE");
bytes32 public constant SET_BOND_CURVE_ROLE =
keccak256("SET_BOND_CURVE_ROLE");

src/VettedGate.sol Outdated
address admin
) {
if (_treeRoot == bytes32(0)) revert InvalidTreeRoot();
if (curveId == 0) revert InvalidCurveId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should compare to the default bond curve here?

src/interfaces/ICSModule.sol Outdated
error ZeroRewardAddress();
error EarlyAdoptionNotSet();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

src/interfaces/ICSModule.sol Outdated
@@ -115,7 +115,7 @@ interface ICSModule is IQueueLib, INOAddresses, IAssetRecovererLib {

event PublicRelease();
event KeyRemovalChargeSet(uint256 amount);

event EarlyAdoptionSet(address earlyAdoption);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

vgorkavenko
vgorkavenko reviewed Jan 10, 2025
View reviewed changes
script/DeployMainnet.s.sol
config
.earlyAdoptionTreeRoot = 0x359e02c5c065c682839661c9bdfaf38db472629bf5f7a7e8f0261b31dc9332c2; // See the first value in artifacts/mainnet/early-adoption/merkle-tree.json
config.earlyAdoptionBondCurve = new uint256[](2);
.vettedGateTreeRoot = 0x359e02c5c065c682839661c9bdfaf38db472629bf5f7a7e8f0261b31dc9332c2; // See the first value in artifacts/mainnet/early-adoption/merkle-tree.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is room for frontrunning anyway. if somebody decides to claim EA before CSM plug-in the gate for v2

src/CSModule.sol
Comment on lines -68 to +72
ICSEarlyAdoption public earlyAdoption;
/// @dev deprecated
address internal _earlyAdoption;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we add a test for checking storage? just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what this test should test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the storage won't corrupted after update

src/VettedGate.sol Show resolved Hide resolved
src/abstract/CSBondCurve.sol
/// @inheritdoc ICSBondCurve
function getCurveInfo(
uint256 curveId
) public view returns (BondCurve memory) {
CSBondCurveStorage storage $ = _getCSBondCurveStorage();
if (curveId > $.bondCurves.length - 1) revert InvalidBondCurveId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think would be better to have _curveExists(CSBondCurveStorage _storage) to not to read the storage twice

src/interfaces/ICSModule.sol
/// @param publicKeys Public keys to submit
/// @param signatures Signatures of `(deposit_message_root, domain)` tuples
/// https://github.com/ethereum/consensus-specs/blob/v1.4.0/specs/phase0/beacon-chain.md#signingdata
/// @notice Permissioned method to add a new Node Operator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would add here role name that can call the method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually don't add the role name
added an explainer in the comment

src/interfaces/ICSModule.sol Outdated
@@ -275,13 +220,15 @@ interface ICSModule is IQueueLib, INOAddresses, IAssetRecovererLib {

/// @notice Add new keys to the existing Node Operator using stETH as a bond
/// @notice Due to the stETH rounding issue make sure to make approval or sign permit with extra 10 wei to avoid revert
/// @param from Sender address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// @param from Sender address
/// @param from tokens sender address

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to add a little more clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in other way

@skhomuti
Merge remote-tracking branch 'origin/develop' into gates-contracts
c471b8e 
# Conflicts:
#	script/DeployBase.s.sol
#	script/fork-helpers/SimulateVote.s.sol
@skhomuti
tests: update CSModule tests
1b8b3d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgusakov dgusakov dgusakov requested changes

@vgorkavenko vgorkavenko vgorkavenko left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skhomuti @vgorkavenko @dgusakov