feat: saner accounting logic #2
Conversation
| @@ -2,7 +2,7 @@ | |||
| pragma solidity ^0.8.25; | |||
|
|
|||
| import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | |||
| import {IFoxStaking} from "./IFoxStaking.sol"; | |||
| import {IFoxStaking, StakingInfo} from "./IFoxStaking.sol"; | |||
| import {console} from "forge-std/Script.sol"; | |||
|
|
|||
| contract FoxStaking is IFoxStaking { | |||
There was a problem hiding this comment.
- for the mappings, no need to have them private, use public so solidity will allow helper functions / getters
- consider consolidating into a struct, so we only have to index once to get the data for a user
- COOLDOWN_PERIOD should not be constant, add priviledged setter so governance can modify
- OZ owner which will be set to the DAO msig after deployment
There was a problem hiding this comment.
https://github.com/shapeshift/rFOX/pull/2/files#diff-e687f106fe1b3606e04f72cd2e69cc2b1836e3bb0d8f411a12a1501947b9ce6bR17 - include rune addy (indexed) in the Staked event
There was a problem hiding this comment.
https://github.com/shapeshift/rFOX/pull/2/files#diff-e687f106fe1b3606e04f72cd2e69cc2b1836e3bb0d8f411a12a1501947b9ce6bR18 - Unstake refactored to request withdraw or however we want to make that more clear.
There was a problem hiding this comment.
IE consistent across function call, event, and mapping / struct values
| @@ -38,26 +38,18 @@ contract FoxStaking is IFoxStaking { | |||
|
|
|||
There was a problem hiding this comment.
function stake(uint 256) and function stake(uint256, string rune addy)
There was a problem hiding this comment.
stake(uint256 amt) {
require(runePairingAddresses[msg.sender] != "")
stake(amt, runePairingAddresses[msg.sender])
}
There was a problem hiding this comment.
https://github.com/shapeshift/rFOX/pull/2/files#diff-e687f106fe1b3606e04f72cd2e69cc2b1836e3bb0d8f411a12a1501947b9ce6bR30 - should confirm that whatever token we are using reverts vs returns false here.
There was a problem hiding this comment.
Note, while we can do overload, we cannot call an overloaded function from another arity variant - went back to the previous implementation of stake (not overloaded) taking a runeAddress: 46718e8
There was a problem hiding this comment.
maybe I oversimplified this in my above comment, it might actually look more like
private _stake(amt) {
do the staking logic
}
external stake(amt) {
// check we have a rune address assigned and then call _stake(amt)
}
external stake(amt, runeAddy) {
// assign new rune addy and then call _stake(amt)
}
also fine with your implementation
| event Unstake(address indexed account, uint256 amount); | ||
| event Withdraw(address indexed account, uint256 amount); | ||
| event SetRuneAddress(address indexed account, string newRuneAddress); | ||
| event SetRuneAddress(address indexed account, string indexed newRuneAddress); |
There was a problem hiding this comment.
Not sure on this one, but do we want to also emit the oldRuneAddress if we are changing it.
There was a problem hiding this comment.
That would definitely make sense for completeness, nice one
|
|
||
| constructor(address foxTokenAddress) { | ||
| foxToken = IERC20(foxTokenAddress); | ||
| } | ||
|
|
||
| function stake(uint256 amount) external { | ||
| function stake(uint256 amount, string memory runeAddress) external { | ||
| require(bytes(runeAddress).length > 0, "Rune address cannot be empty"); |
There was a problem hiding this comment.
can we do better than this? I would imagine rune addresses are specific length / number of bytes so we might be able to
- use something like bytes32 depending on thorchain address structure
- verify the input is alteast the correct number of bytes?
There was a problem hiding this comment.
As long as we don't handle THORnames as valid addresses, we can absolutely do this. Will just need to ensure tests use the right format and we're good
| @@ -38,26 +38,18 @@ contract FoxStaking is IFoxStaking { | |||
|
|
|||
There was a problem hiding this comment.
maybe I oversimplified this in my above comment, it might actually look more like
private _stake(amt) {
do the staking logic
}
external stake(amt) {
// check we have a rune address assigned and then call _stake(amt)
}
external stake(amt, runeAddy) {
// assign new rune addy and then call _stake(amt)
}
also fine with your implementation
See #1 (comment)
This PR:
requestWithdrawactually doesn't do any complex logic of "withdraw request resets the previous one under x condition, or replaces it under y condition" but instead does some simple accounting i.e decreases the staking balance and increases the unstaking balance.StakingInfostruct containing the staking balance, unstaking balance, and cooldown expiry timestamp, and astakingInfo()view fn for itcooldownInfo()since this data is now exposed bystakingInfo()balanceOfnow behaves as a regularbalanceOfi.e "for a given address, give me the amount of tokens I have in the smart contract, regardless of the ramifications of them being still staked and earning rewards, or in the process of being unstaked, with no rewards being earned"