-
Notifications
You must be signed in to change notification settings - Fork 16
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
added rough support for mirror.finance liquidity pools on terra money #25
base: master
Are you sure you want to change the base?
Conversation
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.
Functionally it looks great, please also take a look at some Python formatting conventions or tools - I recommend black.
blockapi/api/__init__.py
Outdated
@@ -30,3 +30,5 @@ | |||
from blockapi.api.tzstats import * | |||
from blockapi.api.zchain import * | |||
from blockapi.api.zensystem import * | |||
|
|||
from blockapi.api.mirror import * |
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.
All the previous imports are sorted alphabetically, could you please do the same?
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.
Yep, no problem. The whole code is just a prototype.
blockapi/api/mirror.py
Outdated
from blockapi.services import BlockchainAPI | ||
|
||
|
||
class MirrorApi(BlockchainAPI): |
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 are extending the functionality of TerraMoneyApi
and copying some of its methods and attributes verbatim.
It is understandable you do not want to modify that class on the first try, but you could subclass it.
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 can extend TerraMoneyApi, subclass it I guess. I wasn't sure if it wont brake the testing mechanism(need to have a look) and if to have one base_url and testnet_url in the singleton api class is generally a good idea. Just not sure here. Point is I need two api servers (lcd.terra.dev a and fcd.terra.dev) - suggestions? Adding it to the class would not work with url_build mechanism. Its just a prototype.
blockapi/api/mirror.py
Outdated
class MirrorApi(BlockchainAPI): | ||
|
||
|
||
symbol = 'LP' |
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.
This symbol is incorrect - LUNA
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.
Not sure either. LUNA makes sense for the getbalance from terraMoneyApi. the amount that fetch_pool_balances return is in LP(it is not a coin though)...
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.
well, Mirror service is not blockchain, so in this case symbol
doesn't make sense
but if there have to be one, LUNA
is closer because LP
is not token, it's just prefix for LP
tokens
btw base currency in Mirror is UST
(USD stable coin)
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.
Ok, lets put there LUNA
it is, settled! :) On the other hand - mmm you call get_balance you get a number without ticker in reality denominated in LP
an you think that if you get a number you get the ticker to it by reading the attribute in the api (I would).
blockapi/api/mirror.py
Outdated
|
||
|
||
symbol = 'LP' | ||
base_url = 'https://tequila-lcd.terra.dev' |
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.
base_url
is not correct either - this is for testnet.
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.
sure, its a prototype....
blockapi/api/mirror.py
Outdated
'umnt': 'MNT' | ||
} | ||
|
||
liqudity_pool_suffix = "LP" |
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 use this variable only once, is it really necessary to define it?
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 can put it just into the evaluation + "LP". I just expected this one to change, so others don't have to search it for too long.
blockapi/api/mirror.py
Outdated
contracts = self.get_contracts() | ||
symbol = contract_addr | ||
|
||
for contract in contracts: |
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.
Can be rewritten more succintly as
for contract, info in contracts.items():
if info['token'] == contract_addr:
symbol = info['symbol']
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.
Definitely better, thx ;)
symbol = contracts[contract]['symbol'] | ||
|
||
if (symbol == contract_addr): | ||
ex = Exception('Could not get symbol based on token address! Check the local copy of whitelisted contracts if it is up to date!') |
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.
Assignment ex = Exception(...)
is not neccessary, just raise Exception(...)
. Also, would it make sense to make a specific exception for this case? Subclassing Exception
?
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.
Definitely. About the exceptions - I have seen this somewhere else in blockapi making instead of exception of the unknown ticker a string "unknown" in the api output. What about contract address from API instead of the ticker? Will there be ETL of that data? What is better? Inconsistency in archived data (with further possibility to reconstruct what ticker it really was from the contract address) or no data because it would crash? I would also crossvalidate the input data - that the token balance I requested is the one I get in the response from lcd.terra.dev - if that wont fit? Exception?
blockapi/api/mirror.py
Outdated
|
||
# loop through the pool contracts and construct the response | ||
# that consist of pool balances of individual pools | ||
for contract in contracts: |
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.
Again, this iteration can be made more readable by for contract, info in contracts.items():
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.
Definitely
blockapi/api/mirror.py
Outdated
return response | ||
|
||
@classmethod | ||
def _get_symbol(cls, denom): |
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.
As discussed at the beginning, this is a verbatim copy of TerraMoneyApi
.
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.
It s a proto. Having code duplicities is baad :)
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 would also encapsulate in the class some of the methods i wrote that don't need to be public.
max_items_per_page = 100 | ||
page_offset_step = 1 | ||
|
||
supported_requests = { |
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.
To also support the requests of TerraMoneyApi
, you can do
supported_requests = {
**TerraMoneyApi.supported_requests,
**{
'get_mirror_pool_balance': '/wasm/contracts/{lp_token_contract}/store?query_msg={{ "balance" : {{ "address" : "{address}" }} }}',
'get_mirror_pool': '/wasm/contracts/{pair_contract}/store?query_msg={{ "pool": {{ }} }}'
}
}
with proper formatting of course :)
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 am not sure what you mean. You mean to put the endpoint and the get parameter after "?" in one string? okok :)
No description provided.