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

Feat: Adding third party stakes using the address of the user #84

Merged
merged 14 commits into from
Oct 1, 2024

Conversation

ogarciarevett
Copy link
Contributor

@ogarciarevett ogarciarevett commented Sep 28, 2024

This pull request introduces significant changes to the staking contract system, primarily focused on enhancing the staking functionality and updating the test cases accordingly. The most important changes include the addition of a new interface for staking operations, modifications to the staking functions to include a positionHolder parameter, and updates to the test cases to reflect these changes.

Enhancements to Staking Functionality:

  • web3/contracts/interfaces/IStaker.sol: Added a new interface IStaker that defines various staking-related functions and events, including createPool, updatePoolConfiguration, stakeNative, stakeERC20, stakeERC721, stakeERC1155, initiateUnstake, and unstake.

Modifications to Staking Functions:

  • web3/contracts/staking/Staker.sol: Updated the staking functions (stakeNative, stakeERC20, stakeERC721, stakeERC1155) to include a positionHolder parameter, allowing the specification of the position holder address. This change also includes updates to the corresponding _mint and emit statements to use positionHolder instead of msg.sender. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]

Updates to Access Control:

  • web3/contracts/staking/Staker.sol: Modified the initiateUnstake and unstake functions to allow the pool administrator to initiate and complete unstaking operations, in addition to the position owner. [1] [2] [3]

Test Case Updates:

Miscellaneous:

Copy link

octane-security-app-dev bot commented Sep 28, 2024

Summary by Octane

New Contracts

  • IStaker.sol: A comprehensive staking smart contract interface facilitating pool creation, configuration, staking of various tokens (Native, ERC20, ERC721, ERC1155), and managing positions with associated events for tracking key actions.

Updated Contracts

  • tokens.sol: Updated to allow compatibility with Solidity version 0.8.24.
  • Staker.sol: The smart contract now allows staking operations to specify a different holder for the position tokens and permits pool administrators to initiate or complete unstaking on behalf of position owners.

🔗 Commit Hash: d298001

@ogarciarevett ogarciarevett marked this pull request as draft September 28, 2024 09:47
Copy link

octane-security-app-dev bot commented Sep 28, 2024

Overview

Vulnerabilities found: 1                                                                                
Severity breakdown: 1 High

Detailed findings

web3/contracts/staking/Staker.sol

  • Review potential Ether or Token Leaking issue that is exposed in the unstake function

🔗 Commit Hash: d298001
🛡️ Octane Dashboard: All vulnerabilities

@ogarciarevett ogarciarevett marked this pull request as ready for review September 28, 2024 14:21
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

Left several questions.

web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/data.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

Left some comments.

web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/contracts/staking/Staker.sol Outdated Show resolved Hide resolved
web3/test/Staker.test.2.ts Outdated Show resolved Hide resolved
web3/test/Staker.test.3.ts Show resolved Hide resolved
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

Two small changes on emission of Unstaked and UnstakeInitiated events.

web3/contracts/staking/Staker.sol Show resolved Hide resolved
zomglings
zomglings previously approved these changes Sep 30, 2024
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

lgtm

(pending event checks for unstake and initiateUnstake in the new tests)

@ogarciarevett feel free to merge once you've added those tests. Thanks for the contribution.

Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

lgtm

@zomglings zomglings merged commit 478d253 into main Oct 1, 2024
2 checks passed
@zomglings zomglings deleted the feat/allow-third-party-stakes branch October 1, 2024 05:32
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.

4 participants