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

CALL_EXCEPTION on array getter #1058

Closed
cruzdanilo opened this issue Sep 21, 2020 · 18 comments
Closed

CALL_EXCEPTION on array getter #1058

cruzdanilo opened this issue Sep 21, 2020 · 18 comments
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@cruzdanilo
Copy link

maybe i'm missing something here, but i cannot call an array getter successfully with an index different from 0.

my code:

const swap = new Contract(pool.curve.address, curvefi.curveAbi, provider);
console.log(await swap.coins(0)); // works ok
console.log(await swap.coins(1)); // Error: call revert exception (method="coins(int128)", errorSignature=null, errorArgs=[null], reason=null, code=CALL_EXCEPTION, version=abi/5.0.5)

getter abi:

{
 "name": "coins",
 "outputs": [
  {
   "type": "address",
   "name": "out"
  }
 ],
 "inputs": [
  {
   "type": "int128",
   "name": "arg0"
  }
 ],
 "constant": true,
 "payable": false,
 "type": "function",
 "gas": 2040
}

contract: https://etherscan.io/dapp/0x45f783cce6b7ff23b2ab2d70e416cdb7d6055f51#readContract

@zemse
Copy link
Collaborator

zemse commented Sep 21, 2020

This seems like a call exception when your array goes out of bounds. Probably your array has just 1 element so that explains that only 0 is working for you.

@cruzdanilo
Copy link
Author

@zemse this is not the case. here is the expected response, from the same contract:
Screen Shot 2020-09-21 at 21 30 13

@zemse
Copy link
Collaborator

zemse commented Sep 21, 2020

Oh. But your code looks fine. I just have a guess that your provider might not be synced to the point where it got the second element, you can check block numbers to confirm this case. If the block number is fine, then this might need to be debugged further.

@cruzdanilo
Copy link
Author

@zemse this array has 4 elements since its compilation (it's hardcoded).

@yuetloo
Copy link
Collaborator

yuetloo commented Sep 21, 2020

I was able to get a result by passing 1 as the input using infura and default provider.

const ethers = require('ethers');

// main network
const provider = new ethers.providers.InfuraProvider();
//const provider = ethers.getDefaultProvider();
const abi = [ {
    "name": "coins",
    "outputs": [{ "type": "address", "name": "out" }],
    "inputs": [{ "type": "int128", "name": "arg0" }],
    "constant": true,
    "payable": false,
    "type": "function",
    "gas": 2160
  } ];

const contractAddress = "0x45F783CCE6B7FF23B2ab2D70e416cdb7D6055f51";
const contract = new ethers.Contract(contractAddress, abi, provider);
contract.coins(1).then(res => console.log(res)).catch(err=> console.log("error", err));
//0xd6aD7a6750A7593E092a9B218d66C0A814a3436e

@cruzdanilo
Copy link
Author

@yuetloo thanks :)
the gas amount was wrong on my abi. should this cause this kind of problem?

@ricmoo
Copy link
Member

ricmoo commented Sep 21, 2020

What is the required gas amount? That will definitely cause a CALL_EXCEPTION (out-of-gas) if you aren’t giving it enough gas to execute...

@cruzdanilo
Copy link
Author

it's a read-only function and the CALL_EXCEPTION says nothing about gas. it also works fine for the element 0.

@yuetloo
Copy link
Collaborator

yuetloo commented Sep 23, 2020

@cruzdanilo , I got my ABI from etherscan. If I remove gas from the abi, my example works too. ethers uses the abi gas when provided.

From my testing, different node gives different error when a low gas amount is provided.

  • infura:
    call revert exception (method="coins(int128)", errorSignature=null, errorArgs=[null], reason=null, code=CALL_EXCEPTION, version=abi/5.0.5

  • alchemy and Cloudflare
    {"jsonrpc":"2.0","id":42,"error":{"code":-32000,"message":"out of gas"}}

  • etherscan sometimes works sometimes timeout
    Error: timeout

@ricmoo
Copy link
Member

ricmoo commented Sep 23, 2020

it's a read-only function and the CALL_EXCEPTION says nothing about gas. it also works fine for the element 0.

It depends what your contract does. If it uses a for-loop, 0 might work because it is early in the loop and there is enough gas. Where as iterating over the further elements on the array each consume more gas. The bounds on the gas that work compared to the gas that doesn’t work are very close, so this could likely be why 0 works and other don’t.

Does that make sense?

@ricmoo
Copy link
Member

ricmoo commented Sep 23, 2020

Actually. This might be something I need to address in ethers.

I currently add 21000 gas to the gas cost in the ABI, but I should compute the intrinsic gas cost of sending the data in the transaction. i.e. the cost of sending a byte in data that is 0, is cheaper than the cost of sending non-zero. So the 1 costs more. Ethers can account for this. It just isn’t now.

Vyper is much more accurate when estimating gas costs, which is why it’s on the edge and this hasn’t come up before.

I’ll look into this soon.

@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. labels Sep 23, 2020
@ricmoo
Copy link
Member

ricmoo commented Sep 26, 2020

I've added logic on my local working copy that incorporates the transaction intrinsic gas, which fixes this.

This issue does bring up an interesting consequence of gas reprices during hard-forks, which is that ABIs will need to be updated as new hard-forks are released.

I still have some build things to figure out with the new scripts, but should be able to get this out this week.

As a work around for now, you can increase the gas property in your ABI until this change is published.

Thanks! :)

@ricmoo
Copy link
Member

ricmoo commented Sep 26, 2020

As a note, this is the code I use to compute the intrinsic gas cost (in the contract, all calls have a non-nil to address, so G_create can be ignored):

let intrinsic = 21000;
const bytes = arrayify(data);
for (let i = 0; i < bytes.length; i++) {
    intrinsic += 4;
    if (bytes[i]) { intrinsic += 64; }
}

@ricmoo
Copy link
Member

ricmoo commented Oct 3, 2020

This change has been added in 5.0.15. Please try it out and let me know if you still have issues. :)

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Oct 3, 2020
@alcuadrado
Copy link

Hey @ricmoo, this change broke Buidler, and I'm not really sure what to do about it.

The reason it broke is that a while ago you told me that the best way to set a default gas cost to contracts was by adding it to the ABI, so I did to every abi fragment when the users is using an in process testing network.

The value I add to those fragments is blockGasLimt - 21000, as users don't care about spending too much gas during testing, and that way I forced ethers to always use the blockGasLimit. Now, that value is normally over the block gas limit, so every call fails.

I don't know what to do because I can't have an upper bound on what intrinsic is going to be.

What's your recommendation?

@alcuadrado
Copy link

btw, in the meantime I'll substract an even larger number, like 1M. But it'd be great if I could tell ethers to use an exact gas limit.

Thanks!

@ricmoo
Copy link
Member

ricmoo commented Oct 4, 2020

@alcuadrado I could add the intrinsic cost calculation as a static method, but that isn’t probably the best way to pick a gas limit.

If you wanted some value to “just pick”, if you have access to the block limit is to make choose 10% of that. Which would normally be far greater than the transaction gas limit (not a protocol enforced value, but one that most nodes choose to follow).

For test environments is it jot normally safe to use the standard estimateGas? Or is this for cases the estimation fails?

@ricmoo
Copy link
Member

ricmoo commented Nov 23, 2020

I think that method to get the intrinsic cost is no longer needed (correct me if I'm wrong?). In v6, this will be a property on the Network object, likely called network.getIntrinsicGasLimit(tx: Transaction) or something similar.

So I'm going to close this for now, but please feel free to re-open to continue discussions.

Thanks! :)

@ricmoo ricmoo closed this as completed Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

5 participants