-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce random stake amount #134
Conversation
Before I review this, the change from int -> float implies that we need to agree on something first: does this represent stake in units of Eth, or in units of wei? We need to make sure it's well documented all the way along. |
It's in Eth, gets converted to wei within the submit prediction function: |
I've updated the docstring, could you re-review? @trentmc |
@@ -248,7 +248,7 @@ def soonest_timestamp_to_predict(self, timestamp): | |||
def submit_prediction( | |||
self, | |||
predicted_value: bool, | |||
stake_amount: int, | |||
stake_amount: float, | |||
prediction_ts: int, | |||
wait_for_receipt=True, | |||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In docstring for submit_prediction(), line for "stake_amount", please say that it's in units of Eth not wei.
@@ -141,7 +141,8 @@ def get_prediction( | |||
You need to customize it to implement your own strategy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that the docstring here is good (it says "in units of wei", good).
Can you also make sure that all other docstrings passing in stake (as eth or wei) clearly specify which it is (eth or wei). Thanks!
I'm also wondering: can we unit test this somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9998e76
Added an assert for the stake amount here
Fixes #133
Changes proposed in this PR: