Update StakingOracle.sol #65
Conversation
There was a problem hiding this comment.
@escottalexander thanks a lot! Cool contract, especially to make it work with the blocks and buckets.
Main points to discuss:
- Finality of buckets (see below)
- ETH reward instead of OracleTokens (see below)
- Median instead of average (see below)
- Minimum of reporters for a valid price (maybe just to mention in readme)
- Sybil Attack vulnerability (maybe just to mention in readme)
| // SPDX-License-Identifier: MIT | ||
| pragma solidity >=0.8.0 <0.9.0; | ||
|
|
||
| import "./OracleToken.sol"; |
There was a problem hiding this comment.
Do we actually need a separate token, or should we just use ETH?
We could simply require the oracle owner to maintain enough ETH in the contract.
If we go that route, we might add a function like refillOracleRewards() to top up the balance.
Otherwise:
import {ORA} from "./OracleToken.sol";
There was a problem hiding this comment.
I think a token does make more sense, just because this would require some tokenomics to be sustainable and that is more easily achieved when you have your own token vs ETH. Also this is trying to tip a hat to Chainlink's designs so I think including a token is nice.
| active: true | ||
| }); | ||
| nodeAddresses.push(msg.sender); | ||
| reportPrice(price); |
There was a problem hiding this comment.
Why call reportPrice(price) immediately inside registerNode?
It feels cleaner to separate those concerns and trigger it explicitly later.
| if (msg.value < MINIMUM_STAKE) revert InsufficientStake(); | ||
| if (nodes[msg.sender].active) revert NodeAlreadyRegistered(); | ||
| nodes[msg.sender] = OracleNode({ | ||
| stakedAmount: msg.value, |
There was a problem hiding this comment.
Incase we keep the oracle token: instead of staking ETH we should then also go with the oracle token, I think
There was a problem hiding this comment.
OK, yeah. I like that idea. Thanks!
| /** | ||
| * @notice Slashes a node for giving a price that is deviated too far from the average | ||
| */ | ||
| function slashNode(address nodeToSlash, uint256 bucketNumber, uint256 index) public { |
There was a problem hiding this comment.
We should clarify in the README that prices can still change after each bucket is reported, because outliers may later be removed.
To prevent this, we could introduce finality for each bucket, once a bucket is finalized, its prices can no longer be modified or removed.
A function like finalizeBucket() could enforce this.
| // Remove the price from the sum and count | ||
| bucket.sumPrices -= reportedPrice; | ||
| bucket.countReports--; | ||
| uint256 averagePrice = bucket.sumPrices / bucket.countReports; |
There was a problem hiding this comment.
If there's only 1 reporter in a bucket and you try to slash them, countReports becomes 0, causing a division by zero.
| address nodeAddress = nodeAddresses[i]; | ||
| uint256 reportedPrice = bucket.prices[nodeAddress]; | ||
| if (reportedPrice == 0) continue; | ||
| uint256 averagePrice = (bucket.sumPrices - reportedPrice) / (bucket.countReports - 1); |
There was a problem hiding this comment.
If there's only 1 reporter in a bucket, countReports is 1, causing a division by zero.
| import { StatisticsUtils } from "../utils/StatisticsUtils.sol"; | ||
|
|
||
| contract StakingOracle { | ||
| using StatisticsUtils for uint256[]; |
There was a problem hiding this comment.
What do think using this lib to use its getMedian function, instead of average calculation? Probably you thought of it since the lib is here :)
But needs some changes to logic.
There was a problem hiding this comment.
Whoops. Yes, I think I was using it before your initial feedback and accidentally left it in. The one problem with using it now is that because this has a permissionless oracle set then it should be able to handle any number of nodes whereas this library is infeasible for large arrays. This is why I am using averages that are tallied with each reportPrice to avoid iterating on each report later. It's all about gas efficiency. Thanks for telling me it is still here. I will remove. I am open to your thoughts if you think we could still use the median somehow.
No description provided.