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

refactor: account for staking contract deposit value #186

Merged
merged 16 commits into from
Jul 19, 2024
Merged

Conversation

kupermind
Copy link
Collaborator

  • Account for staking contract deposit value.

@kupermind kupermind requested review from DavidMinarsch and 77ph July 16, 2024 10:59
contracts/staking/StakingFactory.sol Show resolved Hide resolved
contracts/staking/StakingFactory.sol Outdated Show resolved Hide resolved
audits/internal6/README.md Show resolved Hide resolved
contracts/staking/StakingVerifier.sol Show resolved Hide resolved
contracts/staking/StakingVerifier.sol Outdated Show resolved Hide resolved
return false;
}

// Check for time for emissions
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
// Check for time for emissions
// Check for time for emissions. This lets the verifier enforce an upper bound on the emissions length for risk mitigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kupermind confirming that timeForEmissions is related to activity period?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's related to emissionsAmount of instance that requests that amount from the incentive dispenser.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of livenessPeriod.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the relevant getters so a future Verifier can also impose constrains on livenessPeriod?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean add the livenessPeriodLimit variable?

77ph
77ph previously approved these changes Jul 17, 2024
Comment on lines -306 to -311
address implementation = instanceParams.implementation;

// Check that the implementation corresponds to the proxy instance
if (implementation == address(0)) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check is now useless, it was left there before the struct with implementation / deployer / isEnabled existed. The isEnabled is what matters, and if the implementation is zero, the isEnabled is false by default.

return false;
}

// Check for time for emissions
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the relevant getters so a future Verifier can also impose constrains on livenessPeriod?

@kupermind kupermind merged commit 3f80781 into main Jul 19, 2024
2 checks passed
@kupermind kupermind deleted the deposit_limit branch July 19, 2024 08:47
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