Properly handle beefy CLM vaults #10374
Conversation
| get_or_create_evm_token( | ||
| userdb=database, | ||
| evm_address=deserialize_evm_address(vault['earnedTokenAddress']), | ||
| evm_address=deserialize_evm_address(vault['earnContractAddress']), |
There was a problem hiding this comment.
they're not always the same address?
There was a problem hiding this comment.
I think they are always the same address when both are present, but for some types of vaults the earnedTokenAddress key is missing. earnContractAddress however seems to be present for all vaults.
There was a problem hiding this comment.
i think you should add this as a comment also.
There was a problem hiding this comment.
added a comment mentioning that earnContractAddress is present for all vaults. I don't think it would really make sense to mention earnedTokenAddress in the comment though - future readers of the code will not be aware that that is even a key that may be present since we aren't using it anywhere.
| return ZERO_PRICE | ||
|
|
||
| usd_price = ZERO | ||
| for underlying_token, amount in zip(vault_token.underlying_tokens, response, strict=False): |
There was a problem hiding this comment.
is the order guaranteed to be the same?
There was a problem hiding this comment.
hmm... I expect it would be (the underlying tokens are added in the order used in the API which should be the same) - but this does seem a bit risky - the underlying token order could get changed in the db I suppose or the order in the api could be off in some cases.
Changed this to instead pull the token addresses directly from the clm contract where they are guaranteed to be in the correct order. This also avoids needing to set underlying tokens for the clm cow tokens.
867b6c6 to
f328f7a
Compare
| chain_id=evm_inquirer.chain_id, | ||
| )) is None: | ||
| log.error( | ||
| f'Unexpected underlying token {underlying_token} for ' |
There was a problem hiding this comment.
i think this log message is wrong.
There was a problem hiding this comment.
hmm... so actually I guess I should probably use get_or_create_evm_token here - changed this and the log message.
| get_or_create_evm_token( | ||
| userdb=database, | ||
| evm_address=deserialize_evm_address(vault['earnedTokenAddress']), | ||
| evm_address=deserialize_evm_address(vault['earnContractAddress']), |
There was a problem hiding this comment.
i think you should add this as a comment also.
f328f7a to
00c07d1
Compare
| method_name='previewWithdraw', | ||
| arguments=[single_share_raw_amount], | ||
| ) | ||
| else: |
There was a problem hiding this comment.
is this else an error? If yes please log it as such.
There was a problem hiding this comment.
yes, its an error - but since it sets the addresses and amounts to empty tuples this triggers the if below where it logs an error about an unexpected number of tokens & amounts. This way in both the case of this else, and the case where len(call_output) == 2 but the number of items in the call_output entries are off are both handled without needing separate log.errors.
Added a comment mentioning that it triggers the if below with an error logged there.
00c07d1 to
2f9fe79
Compare
|
rotki/test-caching#217 was successfully merged |
Closes https://github.com/orgs/rotki/projects/11/views/3?pane=issue&itemId=122546245
Unfortunately the beefy docs on this don't get very technical, so the approach here is based on what was said in a discussion in the beefy discord.