safe-global / safe-transaction-service

Keeps track of transactions sent via Safe contacts and confirmed transactions. It also keeps track of Ether and ERC20 token transfers to Safe contracts.

MIT License

191 stars 262 forks source link

Better names for balancer pool tokens #215

Closed tschubotz closed 3 years ago

tschubotz commented 3 years ago

Context

We now have better names for Uniswap pool tokens with #196.

Task

Please also use better names for Balancer pool tokens. They currently cannot be differentiated.

Misc

Zerion uses nice names, so I assume the defi-sdk could be used here as well. e.g. https://app.zerion.io/invest?q=%23Balancer

or https://app.zerion.io/0xBc79855178842FDBA0c353494895DEEf509E26bB/overview

Question

Do we have to add each type separately, or would there we a more integrated or automated way? I'm thinking about other tokens such as:

Curve LP: https://app.zerion.io/invest?q=%23Curve
Yearn Vaults: https://app.zerion.io/invest?q=%23yEarn

Uxio0 commented 3 years ago

Do we have to add each type separately, or would there we a more integrated or automated way?

They provide a way to retrieve all the TokenAdapters and from that TokenAdapter I can get the metadata, so I could find a way to get a token metadata with just the address, but I see 2 problems:

Tokens present in 2 adapters (maybe one is the main and other one is just an exchange supporting that token without all the details)
We still have to know what tokens we want to filter in our service, so I don't scrap every token in our list

We could do it the opposite, get all the supported tokens and update them in our list if there's a match, what do you think @tschubotz ?

tschubotz commented 3 years ago

We could do it the opposite, get all the supported tokens and update them in our list if there's a match, what do you think @tschubotz ?

So essentially this would only update tokens our service is aware of already, right? 🤔 I mean, it's a trade off, right? How much effort would that be? From my end I would it would also be fine if we add token "one by one" depending on user feedback and our observations. i.e. for now we only get the names for Balancer pool tokens and the yearn vault tokens from the ticket description and then we create new tickets in case we want to add more token types. Currently, I didn't notice any more so far where we need to act urgently.

Uxio0 commented 3 years ago

We could do it the opposite, get all the supported tokens and update them in our list if there's a match, what do you think @tschubotz ?

So essentially this would only update tokens our service is aware of already, right? 🤔 I mean, it's a trade off, right? How much effort would that be? From my end I would it would also be fine if we add token "one by one" depending on user feedback and our observations. i.e. for now we only get the names for Balancer pool tokens and the yearn vault tokens from the ticket description and then we create new tickets in case we want to add more token types. Currently, I didn't notice any more so far where we need to act urgently.

They are supposed to have a getSupportedTokens method but I couldn't get it to return anything, I need further testing: https://etherscan.io/address/0xaDfc6460233221eCa99daC25d00f98d32eA3989e#readContract

With that method I would be able all the missing tokens on the service and update the names for the existing ones

tschubotz commented 3 years ago

They are supposed to have a getSupportedTokens method but I couldn't get it to return anything, I need further testing: https://etherscan.io/address/0xaDfc6460233221eCa99daC25d00f98d32eA3989e#readContract

I'm not 100% sure myself, how it works. They put together a string and then convert to bytes32. e.g. 0x556e697377617020563200000000000000000000000000000000000000000003 is the protocol name for https://etherscan.io/address/0x00A6ACb01a3d54ee8139FcAAF839A40eF9d4F888`

https://github.com/zeriontech/defi-sdk/blob/683693d849cf72ab6629be5c5d57005fa1bc5120/test/interactiveAdapters/UniswapV2AssetInteractiveAdapter.js also has some info on this.

Uxio0 commented 3 years ago

It looks like they always set supportedTokens to [] when adding or updating an adapter:

https://etherscan.io/tx/0xbf0c91071a4dfd4b816c1d9ceea0a41e7c1b8a3bd80c4fb234f937e08064e30e

Uxio0 commented 3 years ago

@rmeissner Is it possible that we are missing a way to enumerate the tokens?

rmeissner commented 3 years ago

I did not read all the docs (https://github.com/zeriontech/defi-sdk/wiki) but there are cases where you cannot iterate over all protocol tokens (this depends on the protocol).

See https://github.com/zeriontech/defi-sdk/wiki/Interacting-v2

The function iterates only over the selected tokens for the selected adapter. This is the preferred way of getting balances for pools like Uniswap V1 and Balancer (as they do not have any supported tokens in the registry).

rmeissner commented 3 years ago

Proposed quick fix by @tschubotz is to manually add the top 10 tokens to known addresses

This would allow us to focus on other issues for now

tschubotz commented 3 years ago

This is a list of BPT tokens that I think we should add until we have a proper fix: https://docs.google.com/spreadsheets/d/1JiqsPsufvvGKkzV25pq_BX0XorBUgPehy1VTMFyxe9E/edit#gid=1089514796 Does that work?

Uxio0 commented 3 years ago

Sure, are you adding them yourself or should I code a script to import them from a CSV?

tschubotz commented 3 years ago

By adding myself you mean I would add them to staging mainnet and then you mirror to prod again?

Uxio0 commented 3 years ago

By adding myself you mean I would add them to staging mainnet and then you mirror to prod again?

Yes 😃

Uxio0 commented 3 years ago

Thanks to @tschubotz names are already there https://safe-transaction.mainnet.gnosis.io/api/v1/tokens/?symbol=BPT