Add SnapshotStakingPool and SignedSnapshotStakingPool
#2
Conversation
There was a problem hiding this comment.
Mainly commented on the EIP712 signature mechanism.
I might be misunderstanding the reason why we are doing this, but from a security / cryptography perspective the current way we use it is superflous. (see comments below).
Generally snapshot / staking logic LGTM, also assuming that it didn't change significantly from my last review.
| import {ISignedSnapshotStakingPool} from "../interfaces/staking/ISignedSnapshotStakingPool.sol"; | ||
|
|
||
| import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; | ||
| import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; |
There was a problem hiding this comment.
Great find of this SignatureChecker library with both ECDSA and SC signature support. Just what we needed 👍
|
|
||
| /* EVENTS */ | ||
|
|
||
| event DistributorChanged(address newDistributor); |
There was a problem hiding this comment.
Super small nit. Just for completeness sake we might want to add 1 line docs for each event.
There was a problem hiding this comment.
added here e68a09d
src/staking/SnapshotStakingPool.sol
Outdated
| function accrue(uint256 amount) external nonReentrant onlyDistributor { | ||
| require(amount > 0, "Cannot accrue 0"); | ||
| require(totalSupply() > 0, "Cannot accrue with 0 staked supply"); | ||
| require(canAccrue(), "Snapshot delay not passed"); |
There was a problem hiding this comment.
Since we are using solidity > 0.8 we could use Custom errors that can take extra parameters. (for example in this case you could add the time when the delay would have passed as error parameter.
There was a problem hiding this comment.
added custom errors here 0697f39
src/staking/SnapshotStakingPool.sol
Outdated
| uint256 currentId = _getCurrentSnapshotId(); | ||
| uint256 lastId = nextClaimId[msg.sender]; | ||
| uint256 amount = rewardOfInRange(msg.sender, lastId, currentId); | ||
| require(amount > 0, "No rewards to claim"); |
There was a problem hiding this comment.
Do we really need to revert here ? Is there any scenario where people might want to claim snapshots without any rewards ? What is the downside of just returning 0 rewards ?
There was a problem hiding this comment.
removed here 0697f39
src/staking/SnapshotStakingPool.sol
Outdated
| /// @inheritdoc ISnapshotStakingPool | ||
| function claimPartial(uint256 startSnapshotId, uint256 endSnapshotId) public nonReentrant { | ||
| require(startSnapshotId >= nextClaimId[msg.sender], "Cannot claim from past snapshots"); | ||
| uint256 amount = rewardOfInRange(msg.sender, startSnapshotId, endSnapshotId); |
There was a problem hiding this comment.
Line 113- 116 seem to be identical with the last couple of lines in claim. Maybe we can factor out to an internal function ?
There was a problem hiding this comment.
cleaned up here 0697f39
| require(startSnapshotId <= endSnapshotId && endSnapshotId <= _getCurrentSnapshotId(), "Cannot claim from future snapshots"); | ||
|
|
||
| uint256 rewards = 0; | ||
| for (uint256 i = startSnapshotId; i <= endSnapshotId; i++) { |
There was a problem hiding this comment.
Interesting that the range here includes both the start and end value. Nothing wrong with that just have to keep it in mind since this might be different to what people are used to.
(Also wondering if doing i < endSnapshotId + 1 might save gas or would compile to the same bytecode)
Regarding gas, also I remember ++i being cheaper than i++ (and functionally equivalent in this case). Although not sure if this is still the case with solidity 0.8
There was a problem hiding this comment.
yes things are a bit wonky with the 1 based indexing of ERC20Snapshot - open to suggestions on what could be more readable
src/staking/SnapshotStakingPool.sol
Outdated
| /// @inheritdoc ISnapshotStakingPool | ||
| function rewardOfAt(address account, uint256 snapshotId) public view virtual returns (uint256) { | ||
| require(snapshotId > 0, "ERC20Snapshot: id is 0"); | ||
| require(snapshotId <= _getCurrentSnapshotId(), "Cannot claim from future snapshots"); |
There was a problem hiding this comment.
Another example where Custom errors with parameters might be helpful.
There was a problem hiding this comment.
added custom errors here 0697f39
pblivin0x
changed the title
SnapshotStakingPoolSnapshotStakingPool and SignedSnapshotStakingPool
No description provided.