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.

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

[PDE-691] staking contract for nYIELD #159

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

[PDE-691] staking contract for nYIELD #159

wants to merge 56 commits into from

Conversation

ungaro
Copy link
Member

@ungaro ungaro commented Jan 28, 2025

What's new in this PR?

add nYield pre-deposit contract

staking/src/nYieldStaking.sol Outdated Show resolved Hide resolved
@ungaro ungaro requested review from qubitcrypto and eyqs February 4, 2025 10:11
@ungaro ungaro marked this pull request as ready for review February 4, 2025 10:11
@ungaro
Copy link
Member Author

ungaro commented Feb 4, 2025

PTAL @qubitcrypto @eyqs
files to review: nest/src/BoringVaultPredeposit.sol & nest/test/BoringVaultPredeposit.t.sol

Copy link
Member

@qubitcrypto qubitcrypto left a comment

Choose a reason for hiding this comment

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

lgtm, I provided some minor optimizations, feel free to take them or leave them.

nest/script/DeployBoringVaultPredeposit.s.sol Show resolved Hide resolved
staking/src/ReserveStaking.sol Show resolved Hide resolved
staking/src/RWAStaking.sol Show resolved Hide resolved
Copy link
Member

@qubitcrypto qubitcrypto left a comment

Choose a reason for hiding this comment

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

I'm concerned with the initialize() and reinitialize() functions, see my comments. Therefore I'm changing the status to "Request changes".

/**
* @notice Initialize the contract.
* @param timelock The timelock contract address.
* @param owner The owner address.
Copy link
Member

Choose a reason for hiding this comment

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

Be explicit who the owner is? the deployer? a multisig contract address?

function reinitialize(address multisig, TimelockController timelock) public reinitializer(2) onlyRole(ADMIN_ROLE) {
BoringVaultPredepositStorage storage $ = _getBoringVaultPredepositStorage();
$.multisig = multisig;
$.timelock = timelock;
Copy link
Member

Choose a reason for hiding this comment

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

At this point, the contract's super admin role and admin role is still the "owner", potentially the deployer.

All the following functions requires "ADMIN_ROLE" which has nothing to do with this multisig we set here.

If I understand our intention correctly, we must revoke and grant roles with reinitialize(), and emitting events accordingly:


    // Transfer administrative roles
    address oldAdmin = getRoleMember(DEFAULT_ADMIN_ROLE, 0);
    _revokeRole(DEFAULT_ADMIN_ROLE, oldAdmin);
    _revokeRole(ADMIN_ROLE, oldAdmin);
    
    _grantRole(DEFAULT_ADMIN_ROLE, multisig);
    _grantRole(ADMIN_ROLE, multisig);
    
    emit MultisigSet(multisig);
    emit TimelockSet(address(timelock));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants