Skip to content
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

New eth-scan broken for some tokens #260

Open
LefterisJP opened this issue Dec 8, 2022 · 3 comments
Open

New eth-scan broken for some tokens #260

LefterisJP opened this issue Dec 8, 2022 · 3 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@LefterisJP
Copy link

LefterisJP commented Dec 8, 2022

The problem

At rotki we use the old eth-scan version: https://etherscan.io/address/0x86F25b64e1Fe4C5162cDEeD5245575D32eC549db

Tried and true, works every time.

@yabirgb deployed the new one in optimism, so thought we could try and use it in mainnet too.

While trying to accomplish that it turns out that there is some edge-cases for which it's broken.

While for many tokens it works fine and returns True/false plus the uint256 in bytes for the token balance, there is a few tokens where it returns True plus a humongous byte string.

One such token is 0x0e880118C29F095143dDA28e64d95333A9e75A47 (https://etherscan.io/address/0x0e880118C29F095143dDA28e64d95333A9e75A47)

If you try to query balance for any user it spits out the huge byte string. But probably best example is someone who actually owns a bit of it.

So tokensBalance for owner: 0xeE620a0991D57f464aaD452789a4564Ba51245E8 and contracts: 0x0e880118C29F095143dDA28e64d95333A9e75A47

Try via: https://etherscan.io/address/0x08A8fDBddc160A7d5b957256b903dCAb1aE512C5#readContract

Result can be seen below

2022-12-08_15-20

Some extra thoughts

I can't help but feel that the original very first contract is superior in every way. Only downside is that it has view only the EtherBalances and the other calls are not view while they should have been.

But for this new one the following problems exist:

  1. Adding success/false for each underlying token call does not help much. As a consumer app I only care about the balance and not success/false of the balanceOf. If the call failed, I would always assume 0 balance anyway. So this adds unecessary complexity and reduces the amount of possible tokens we can have in a roundtrip to a node due to size limitations.
  2. Having success/false also for etherBalances does not make sense since it's always true. So it takes up extra space and reduces the amount of possible addresses we can have in a roundtrip to a node due to size limitations.
  3. Having the result in bytes. I really don't get this. The original contract had it in uint256 which was exactly what you need. We are always dealing with uint256. Either token balances or eth balances. What was the reason to do this? This adds complexity in the consumer side, as we need to convert from byte string to int for every single token. Slows things down quite a lot considering our calls are in chunks of thousands of tokens.

It's quite possible I am missing something, but I would love some explanation into the thinking that went into these changes.

Explanation on the reduction of amount of possible addresses: Same code that was querying exact same amount of tokens for a given address fails both with etherscan proxy (they close the connection) and with my own node (times out). Works fine with old contract.

If with the new contract I send less addresses in each batch then it works. So in the end the new contract forces us to make more calls to a node.

@Mrtenz Mrtenz self-assigned this Dec 8, 2022
@Mrtenz Mrtenz added bug Something isn't working help wanted Extra attention is needed labels Dec 8, 2022
@Mrtenz
Copy link
Collaborator

Mrtenz commented Dec 8, 2022

Thanks for bringing this up! I'm not sure why this happens for this particular contract. I'm not familiar with Vyper, but I will try and look more into this. Any suggestions as to why this might happen are much appreciated too.

It could potentially be solved by using just the first 32 bytes, which is what we do in the JavaScript library:

const value = typeof next === 'bigint' ? next : toNumber(next[1].slice(0, 32));

Regarding the old versus new contract: We ran into similar issues with certain contracts, like non-ERC-20 compliant proxy contracts, where the balanceOf function isn't a view function. This works fine with eth_call, but not with Solidity's contract.call. We went through some iterations, and ended up with specifying whether the call succeeded or not, which is used by the JS library to re-fetch balance if necessary. I don't remember the exact details, but this seemed like the best way to solve it at the time.

We also had issues with the call running out of gas, due to a non-compliant token using up the entire gas limit. I believe this is because a call to a non-view function uses up all of the gas.

Some of the contracts that were causing issues before:

Having the result in bytes. I really don't get this. The original contract had it in uint256 which was exactly what you need. We are always dealing with uint256. Either token balances or eth balances. What was the reason to do this?

Again, I'm not a 100% sure what the exact reason for this was, but I think it may have to do with reading revert reasons.

I'm happy to accept any contributions!

@LefterisJP
Copy link
Author

This works fine with eth_call, but not with Solidity's contract.call

So when called from inside contracts you had problems, but when called from the node it was all fine, right?

@Mrtenz
Copy link
Collaborator

Mrtenz commented Dec 9, 2022

So when called from inside contracts you had problems, but when called from the node it was all fine, right?

Yes. So in the JavaScript library we check if the call failed, and fetch again by calling eth_call on a node directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants