add predictoor template #749
Conversation
alexcos20
added
Status: DoNotMerge
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Changes: - rail every blockNum function args to slot, instead of revert - payout function never reverts - renamed to ERC20Template3 - events are storing slot, it will be easier to group it in subgraph - truval_submitted it's an enum now, instead of bool - removed external addMinter function - refactor soonest_block_to_predict (there is blockNo diff between get and actual prediction block) - added SettingChanged event
| return (subscription_revenue_at_block[slot]); | ||
| } | ||
|
|
||
| function get_prediction( |
There was a problem hiding this comment.
Rename to get_predval()
Why: to disambiguate from each predictoor's predval, versus the top-level aggregated predval agg_predval.
There was a problem hiding this comment.
This returns entire prediction (predval,stake,predictoor,paid) and not just the predval
| Paying, | ||
| Canceled | ||
| } | ||
| event PredictionSubmitted( |
There was a problem hiding this comment.
Rename to PredvalSubmitted
Why: disambiguate from each predictoor's predval, versus the top-level aggregated predval agg_predval.
There was a problem hiding this comment.
we don't have the predval in the event (would be easy for somebody else to see it), we only have the information that a prediction for slot X has been submited by predictoor Y and he/she staked Z
There was a problem hiding this comment.
Let's rename to SingPredictionSubmitted.
| uint256 indexed slot, | ||
| uint256 stake | ||
| ); | ||
| event PredictionPayout( |
There was a problem hiding this comment.
Rename to PredictoorPayout
Why: more specific / clearer
There was a problem hiding this comment.
A Predictoor can have multiple payouts. It's more clear "Prediction" because then we know that it refers to a certain prediction
There was a problem hiding this comment.
Makes sense.
Then, in line with above, I recommend rename to SingPredictionPayout
| bool trueval, | ||
| Status status | ||
| ); | ||
| struct Prediction { |
There was a problem hiding this comment.
I wonder if we should rename this to PredictionStruct? Will help to disambiguate from predval which it holds.
There was a problem hiding this comment.
This is a prediction.
A prediction is formed of :
- predictoor address
- predval
- stake
- paid (if it was paid or not)
|
TODO: use mixedCase, according to Solidity naming convention |
| @@ -0,0 +1,1150 @@ | |||
| pragma solidity 0.8.12; | |||
There was a problem hiding this comment.
Let's use the labels sing_predval, agg_predval and trueval everywhere.
- And camelcase equivalents when appropriate.
- And "prediction" for a group of data around a "predval" (eg predval, stake, predictoor address)
Details...
Over the weekend I had an idea: to further disambiguate singular predvals from aggregated predvals, add "sing" in front of each predval label. Eg "sing_predval", side-by-side with "agg_predval".
Note how I use "predval" and "trueval" without spaces, underscore, or camelcase. That's on purpose, it's just one word. Just like we use "datatoken" not "data token". It reinforces the mental model of predicted vs true, and reduces chance of error.
I thought about doing the same with "sing" vs "agg" too, ie "singpredval", "aggpredval", "trueval". But it's too hard cognitively. So let's just stick with "predval" (having two variants "sing_predval" and "agg_predval") and "trueval".
All the variable labels should reflect this: sol, py, js, and GDoc.
We should rename variables everywhere accordingly. It might be annoying to have to do right now, but it will save us a lot of grief going forward.
I won't give individual comments on this below, it'd be annoying to track. Just make the changes. When done, ping me, I'll double-check.
| Paying, | ||
| Canceled | ||
| } | ||
| event PredictionSubmitted( |
There was a problem hiding this comment.
Let's rename to SingPredictionSubmitted.
| uint256 indexed slot, | ||
| uint256 stake | ||
| ); | ||
| event PredictionPayout( |
There was a problem hiding this comment.
Makes sense.
Then, in line with above, I recommend rename to SingPredictionPayout
| bool trueval, | ||
| Status status | ||
| ); | ||
| struct Prediction { |
…on_2 Feature/predictoor iteration 2
For this epic "[Predictoor BE] Ship MVP" -- Add DT3 to contracts repo
Discussion on whether to merge to main: predictoor-pm#18