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

Remove Gas Estimates from ABI #2151

Closed
fubuloubu opened this issue Sep 13, 2020 · 9 comments
Closed

Remove Gas Estimates from ABI #2151

fubuloubu opened this issue Sep 13, 2020 · 9 comments
Labels
bug Bug that shouldn't change language semantics when fixed. Easy Pickings Used to denote issues that should be easy to implement

Comments

@fubuloubu
Copy link
Member

https://twitter.com/_prestwich/status/1304986329604710401?s=20

Basically, since gas costs change all the time, we can't rely on the estimates that a particular version of Vyper uses. So, we should just get rid of this "feature"

NOTE: we'll still keep an upper bound on gas usage, for use with optimizations and to prevent gas attacks

@fubuloubu fubuloubu added bug Bug that shouldn't change language semantics when fixed. Easy Pickings Used to denote issues that should be easy to implement labels Sep 13, 2020
@fubuloubu fubuloubu added this to the v0.3.0 Release milestone Sep 13, 2020
@iamdefinitelyahuman iamdefinitelyahuman changed the title Remove Gas Estiamtes from ABI Remove Gas Estimates from ABI Sep 13, 2020
@bitcoinbrisbane
Copy link

I'd like to take a swing at this...

@bitcoinbrisbane
Copy link

Should be as simple as removing this guy? https://github.com/vyperlang/vyper/blob/master/vyper/compiler/output.py#L56

@fubuloubu
Copy link
Member Author

@bitcoinbrisbane it should be that easy, yes! But if you could also try to remove that functionality from the rest of the codebase and the test suite, that'd seal the deal!

@bitcoinbrisbane
Copy link

@bitcoinbrisbane it should be that easy, yes! But if you could also try to remove that functionality from the rest of the codebase and the test suite, that'd seal the deal!

On it

@bitcoinbrisbane
Copy link

43 tests break after removing it, taking a bit of time to refactor those tests.

@Alec1017
Copy link

Hey @bitcoinbrisbane, just curious of there were any updates on the progress of this issue so far?

@bitcoinbrisbane
Copy link

Not good, I got stuck on refactoring the unit tests. I was just thinking about this ticket on the weekend. Can I try again this weekend?

@charles-cooper
Copy link
Member

@bitcoinbrisbane I can help or take over this issue, if you want

@bitcoinbrisbane
Copy link

Sounds good. I need to get back into compilers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug that shouldn't change language semantics when fixed. Easy Pickings Used to denote issues that should be easy to implement
Projects
None yet
Development

No branches or pull requests

4 participants