-
Notifications
You must be signed in to change notification settings - Fork 6
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
Quickstudies #65
base: main
Are you sure you want to change the base?
Quickstudies #65
Conversation
- Limit on-curve liquidity
Limit max on curve liquidity
✅ Deploy Preview for incandescent-kelpie-250f0e canceled.
|
This fixes the scenario_generator module for monte-carlo simulations. |
if str(amount).endswith("%"): | ||
return ( | ||
get_user_balance(state, user_name, tkn_name) | ||
* Decimal(str(amount)[:-1]) |
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.
Same with regards to str(amount)
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.
same response as previous
@@ -187,8 +191,9 @@ def deposit( | |||
Top level logic for deposit actions. | |||
""" | |||
state = self.get_state(copy_type="initial", timestamp=timestamp) | |||
state, tkn_name, user_name = validate_input( | |||
state, tkn_name, user_name, timestamp | |||
tkn_amt = userAmount(state, tkn_name, user_name, tkn_amt) |
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.
You can/should revert this change.
Since tkn_amt
is computed based on state, tkn_name, user_name
, we should first have them validated (i.e., internally amended).
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.
How/why would tkn_amt
be computed based on state, tkn_name, user_name
? This is a BancorDapp
method, meaning it is the intended API for users of this codebase - hence they should be the ones specifying what the tkn_amt
is. The deleted validate_input(..)
did not include tkn_amt
per your recent changes, presumably because the validation would have failed for string amounts with %
signs, however, it makes more sense to simply call validate_input(..)
on the tkn_amt
directly after calling userAmount(...)
(i.e., line 195 in this case). In short, Im suggesting we retain the original validate_input(..)
which includes tkn_amt
, but simply handle this after calling userAmount(...)
.
state, source_token, user_name, timestamp | ||
tkn_amt = userAmount(state, source_token, user_name, tkn_amt) | ||
state, source_token, tkn_amt, user_name = validate_input( | ||
state, source_token, tkn_amt, user_name, timestamp |
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.
Same (you can/should revert this change).
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.
same as above
state, tkn_name, user_name, timestamp | ||
tkn_amt = userAmount(state, tkn_name, user_name, tkn_amt) | ||
state, tkn_name, tkn_amt, user_name = validate_input( | ||
state, tkn_name, tkn_amt, user_name, timestamp |
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.
Same (you can/should revert this change).
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.
same as above
…arch into quickstudies
- changed `tkn_amt: str` to `tkn_amt: Any` - changed camelcase functions to lowercase for simulator python style consistency
- fixed some inconsistent usage
No description provided.