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

add admin only functions #2512

Merged
merged 5 commits into from
Feb 7, 2025
Merged

add admin only functions #2512

merged 5 commits into from
Feb 7, 2025
+174 −22

Conversation

alysiahuggins
Copy link
Contributor

@alysiahuggins alysiahuggins commented Jan 30, 2025 •
edited
Loading

Closes #2310
Sub-issue of #2309

The admin of the contract will be a multisig and will have the ability to update

  • churn rate
  • min stake amount
  • light client address
  • the admin

This PR:

  • adds an admin
  • allows the admin to update variables above

This PR does not:

  • allow the admin to update validator threshold as that logic hasn't been implemented yet
  • determine a max churn rate or max minStakeAmount allowed, but would like to discuss tihs
  • create an emergencyExit functions that only validators can call
  • add a function to withdraw unstaked funds to the contract (e.g. funds that persons sent by mistake)

Key places to review:

  • admin logic and discussion topics mentioned

How to test this PR:

forge test match-contract StakeTable

@alysiahuggins alysiahuggins linked an issue Jan 30, 2025 that may be closed by this pull request
Stake Table: Admin methods #2309
Open
2 tasks
@alysiahuggins alysiahuggins requested a review from 0xkato January 30, 2025 20:37
sveitser
sveitser reviewed Feb 3, 2025
View reviewed changes
contracts/src/StakeTable.sol Outdated
/// @dev The admin cannot be set to the zero address
/// @param _admin The new admin
function updateAdmin(address _admin) external {
if (msg.sender != admin) revert Unauthorized();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it a conscientious decision not to inherit from an existing contract with access control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you know what, i was thinking through it, wondered if this was more gas efficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to Ownable here dbcfeb3

0xkato
0xkato reviewed Feb 3, 2025
View reviewed changes
contracts/src/StakeTable.sol Outdated
@@ -269,7 +292,7 @@ contract StakeTable is AbstractStakeTable {
BN254.G1Point memory blsSig,
uint64 validUntilEpoch
) external override {
uint256 fixedStakeAmount = minStakeAmount();
uint256 fixedStakeAmount = minStakeAmount;

// Verify that the sender amount is the minStakeAmount
if (amount < fixedStakeAmount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this amount variable if we are not using it? Everyone puts in the same amount (minStakeAmount) when calling register().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this out, confirming with @veilgets. Are we doing a fixedAmount and thus the amount they stake has to be that fixedAmount or are we doing a variable amount that must be >= some minAmount?

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're choosing to go with a variable stake, so updated here 2feda04

@sveitser
Copy link
Collaborator

sveitser commented Feb 6, 2025

I think this PR shouldn't close #2309 because it does not address all the items in it (e.g. validator threshold).

@alysiahuggins
Copy link
Contributor Author

I think this PR shouldn't close #2309 because it does not address all the items in it (e.g. validator threshold).

yea i'll create another issue for the validator thresholds, we didn't have consensus whether we were using thresholds until a few days ago from @veilgets

sveitser
sveitser reviewed Feb 7, 2025
View reviewed changes
contracts/test/StakeTable.t.sol
@@ -207,7 +207,7 @@ contract StakeTable_register_Test is Test {
}

function test_RevertWhen_WrongStakeAmount() external {
uint64 depositAmount = 5 ether;
uint64 depositAmount = uint64(stakeTable.minStakeAmount()) - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename test function? Insufficent stake amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, updated 5dcb697

@alysiahuggins alysiahuggins merged commit 6014858 into main Feb 7, 2025
22 checks passed
@alysiahuggins alysiahuggins deleted the 2309-stake-table-admin-methods branch February 7, 2025 04:04
@alysiahuggins alysiahuggins mentioned this pull request Feb 7, 2025
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xkato 0xkato 0xkato left review comments

@sveitser sveitser sveitser approved these changes

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@jbearer jbearer Awaiting requested review from jbearer jbearer is a code owner

@tbro tbro Awaiting requested review from tbro tbro is a code owner

@imabdulbasit imabdulbasit Awaiting requested review from imabdulbasit imabdulbasit is a code owner

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

Successfully merging this pull request may close these issues.

Stake Table: Admin methods Stake Table: Variable Stake Amounts
3 participants
@alysiahuggins @sveitser @0xkato