67P / kredits-contracts

:warning: [MOVED] Smart contracts and JS API for Kosmos Kredits
https://gitea.kosmos.org/kredits/contracts
4 stars 4 forks source link

Fix gasLimit issue that requires manual gasLimit overwrites #132

Closed bumi closed 5 years ago

bumi commented 5 years ago

Currently we set a fixed gasLimit for some/all(?) transaction calls because otherwise we ran into a revert on the network. It seems that metamask can not correctly estimate the gas usage of those calls.

There have been different reports and it is unclear if this is only an issue on the local dev-chain or also on rinkeby or mainnet.

We do not want to have a fixed gasLimit on the transaction calls. Any ethereum node should be able to estimate that gasLimit and we should not need to set that manually to an arbitrary high number.

This is a trouble issue because too many people have already spent way too much time figuring things out.

bumi commented 5 years ago

Here is the transaction that added nick: https://rinkeby.etherscan.io/tx/0xe05f68099298198b07a95607151afa515c6f55affa45722cacb617e126450e41

It has a gasLimit which we set to 350,000. the actual gas usage is 165,786 (47.37%), which results in a transaction fee of 0.000165786 Ether (at a gas price of 1 Gwei)

bumi commented 5 years ago

this could be used as potential test code: https://github.com/bumi/kredits-deployer/blob/master/main.js

no gasLimit is set there:

  1. newInstance: https://github.com/bumi/kredits-deployer/blob/master/main.js#L68
  2. addContributor: https://github.com/bumi/kredits-deployer/blob/master/main.js#L39
bumi commented 5 years ago

we should first update our dependencies (ganache & co.) there have been significant changes, and also a release dedicated to gas estimations: https://github.com/trufflesuite/ganache-core/releases/tag/v2.5.4-beta.2

but that only confirms potential issues with the local network.

maybe somebody can test rinkeby transactions with the above code? (deployed here)

bumi commented 5 years ago

whoever had these problems, could you have a look at the above comments and test it?

raucao commented 5 years ago

If you can give a few more pointers for how to test it, then sure.

bumi commented 5 years ago

did you see the above comments? the mentioned code is deployed here

raucao commented 5 years ago

You asked to have a look at the above comments, so yes, I read the above comments. They don't really say how one would run/test it exactly. If I missed that, then maybe you can just copy and paste that description again. Thank you.

bumi commented 5 years ago

use that app/execute a transaction? click on that "create your kredits org" button in that app (in a browser with metamask connected to rinkeby) - and wait a while until the transaction has 3-4 confirmations)

The used code (linked above) is not setting any gasLimit option. If that works it indicates that a gasLimit option is not necessarily needed (on a public/non-devchain) network.

raucao commented 5 years ago

Yes, how do I use the app and execute a transaction? None of the comments describe what it even does or link to a README or anything. All the links are just to source code.

bumi commented 5 years ago

the link is to a website that has one button? you'd only need to click that button - like you would use the kredits-web app. and the readme also has links to a deployed instance...

image

I feel like this is already becoming again a topic of conflict - if you don't want to look at it, it is fine... I don't have this problem...all my transaction tests worked fine on rinkeby and the next ganache version will also bring changes to the fee estimation. I will not spend more time on this right now.

raucao commented 5 years ago

You could have just cited the link again. I just re-read the topic 3 times, until I finally found the one word that is linked to the deployment. Everything else, which is much more content, didn't link to any app at all.

If someone overreads something repeatedly, then why not just quote whatever you were talking about. Takes two seconds, and no need to discuss at all.

raucao commented 5 years ago

I tried it now and the tx was published. So I guess that means it works.

raucao commented 5 years ago

I think this should still be tested with the helper scripts and in kredits-web, but feel free to close anytime. We can re-open it, if it fails there.

bumi commented 5 years ago

it is cited again. no idea what to do more.

thanks that you tested it. closing this, and let's try to not commit any gasLimit options into the code anymore.