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

Sm 352 seeker staking #172

Merged
merged 7 commits into from
Jun 21, 2024
Merged

Sm 352 seeker staking #172

merged 7 commits into from
Jun 21, 2024

Conversation

Nick95550
Copy link
Contributor

@Nick95550 Nick95550 commented Jun 7, 2024

Implements the interface and contract for Seeker Staking.

Copy link
Collaborator

@JCSanPedro JCSanPedro left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@Nick95550 Nick95550 force-pushed the SM-352-seeker-staking branch from dadef6a to 3b38c2e Compare June 14, 2024 04:31
Copy link
Contributor

@teinnt teinnt left a comment

Choose a reason for hiding this comment

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

Wow awesome!!! Very cool PR!!! I am glad to review this PR! Thanks Nick!!!!!

contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
Comment on lines 136 to 139
StakedErrors err = _isSeekerStakedError(node, seeker.seekerId);
if (err == StakedErrors.NIL) {
_unstakeSeeker(node, seeker.seekerId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I dont think we should auto unstake for them. They should do this themselves, or just transfer the seeker ownership. This code needs a comment to warn users, here or in the @notice comment. But I think we should just revert here instead of auto unstake for the user. It makes the code clearer and be strict for what it should do. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did this on Johns recommendation, I think I tend to agree with your pov though. #172 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the user should have to unstake then stake again (2 transactions) just to move their staked seeker.

I agree that the @notice comment should mention that it will unstake the seeker if it is already staked.

contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
contracts/staking/seekers/SeekerStakingManager.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@teinnt teinnt left a comment

Choose a reason for hiding this comment

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

LGTM!!!! GREAT PR!!!!

@teinnt teinnt merged commit 7cd0e99 into v2/seeker-staking Jun 21, 2024
6 checks passed
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