First draft of staking contract. #5
Conversation
| it("Should stake/unstake tokens for self", async function () { | ||
| const initialBalance = await token.balanceOf(user1.address); | ||
|
|
||
| await staking.connect(user1).stake(stakeAmount, user1.address); |
There was a problem hiding this comment.
issue (non-blocking): I think we should also assert events emission, especially because this gonna be used for the indexers
| await staking.connect(user1).stake(stakeAmount, user1.address); | |
| await expect(staking.connect(user1).stake(stakeAmount, user1.address)).to.emit("Stake", staking).withArgs(...) |
There was a problem hiding this comment.
Added event expectations.
web3/contracts/staking/Staking2.sol
Outdated
| * @title Game7 Staking | ||
| * @author Game7 Engineering Team - [email protected] | ||
| */ | ||
| contract Staking2 { |
There was a problem hiding this comment.
question (non-blocking): I saw that the contract does not have a constructor called. It might be worth having an empty constructor since non-constructed contracts have zero code length, and some contracts use the function isContract before calling. Do we want to have the contract constructed?
web3/contracts/staking/Staking2.sol
Outdated
| return depositsOf[user].length; | ||
| } | ||
|
|
||
| function deposit(address _tokenAddress, uint256 _amount, uint256 _duration, address _receiver) external { |
There was a problem hiding this comment.
suggestion (non-blocking): Since duration is being casted to uint64, i think it can be directly typed to uint64 in the function parameter
| function deposit(address _tokenAddress, uint256 _amount, uint256 _duration, address _receiver) external { | |
| function deposit(address _tokenAddress, uint256 _amount, uint64 _duration, address _receiver) external { |
There was a problem hiding this comment.
Possibly address in future. Seems low priority.
web3/contracts/staking/Staking2.sol
Outdated
| emit Deposited(_tokenAddress, _receiver, msg.sender, _duration, _amount); | ||
| } | ||
|
|
||
| function withdraw(uint256 _depositId, address _receiver) external { |
There was a problem hiding this comment.
suggestion (non-blocking): As a user, I would be stressed over ensuring I am passing the right address as the receiver. I would love to have a function that receives only the deposit ID and withdraws it to the msg.sender. I think we can achieve that with function overloading:
| function withdraw(uint256 _depositId, address _receiver) external { | |
| function withdraw(uint256 _depositId, address _receiver) external { | |
| _withdraw(_depositId, _receiver); | |
| } | |
| function withdraw(uint256 _depositId) external { | |
| _withdraw(_depositId, msg.sender); | |
| } | |
| function _withdraw(uint256 _depositId, address _receiver) external { | |
| .... | |
| } |
There was a problem hiding this comment.
Overloaded both deposit and withdraw.
web3/contracts/staking/Staking2.sol
Outdated
| return depositsOf[user].length; | ||
| } | ||
|
|
||
| function deposit(address _tokenAddress, uint256 _amount, uint256 _duration, address _receiver) external { |
There was a problem hiding this comment.
suggestion (non-blocking): As a user, I would like to know if, in the future, we want to deposit/stake multiple tokens in just one transaction. Or even stake different amounts of the same token for different periods. I want to do that in just one transaction. Maybe we should put the arguments as arrays to support that:
| function deposit(address _tokenAddress, uint256 _amount, uint256 _duration, address _receiver) external { | |
| function deposit(address[] memory _tokenAddresses, uint256[] memory _amounts, uint256[] memory _durations, address[] memory _receivers) external { |
There was a problem hiding this comment.
Possibly, but I don't see a strong need for this right now. I believe the current plan is to incentivize just 3 different locking periods and we're not sure what other tokens we will want users to stake. The need for a batch method seems low.
| require(_amount > 0, "Staking.deposit: cannot deposit 0"); | ||
|
|
||
| IERC20 depositToken = IERC20(_tokenAddress); | ||
| depositToken.transferFrom(msg.sender, address(this), _amount); |
There was a problem hiding this comment.
question (blocking): How will we deal with the native token?
…with no _receiver.
No description provided.