Skip to content
This repository was archived by the owner on Jun 6, 2024. It is now read-only.

fix: proper gas estimation #67

Merged
merged 32 commits into from
Aug 2, 2022

Conversation

ca11ab1e
Copy link
Contributor

@ca11ab1e ca11ab1e commented Jul 15, 2022

What I did

fixes: #52

Ideally, that PR would:

  • fix auto-fee estimation
  • unlock gas estimation on "local" networks.

How I did it

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@ca11ab1e ca11ab1e force-pushed the fix-52-gas-estimation-devnet branch from 025d2fb to dcd66e7 Compare July 15, 2022 12:43
Also unlock gas estimation on the devnet.
@ca11ab1e ca11ab1e force-pushed the fix-52-gas-estimation-devnet branch from dcd66e7 to c75f81e Compare July 15, 2022 12:45
It doesn't make sense to estimate gas for non-signed transactions.
@ca11ab1e ca11ab1e force-pushed the fix-52-gas-estimation-devnet branch from 964e2a0 to c461457 Compare July 15, 2022 16:05
@ca11ab1e

This comment was marked as outdated.

@ca11ab1e

This comment was marked as outdated.

@ca11ab1e ca11ab1e requested a review from antazoey July 21, 2022 08:46
@ca11ab1e

This comment was marked as outdated.

antazoey and others added 3 commits July 28, 2022 17:28
@ca11ab1e ca11ab1e force-pushed the fix-52-gas-estimation-devnet branch from 8323fda to f6a9309 Compare July 29, 2022 09:59
@ca11ab1e

This comment was marked as outdated.

**NOTE**: Currently, the gas price is fixed to always be 100 gwei.
"""
return self.conversion_manager.convert("100 gwei", int)
return self.current_gas_price
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to get the value from a block:

https://github.com/software-mansion/starknet.py/pull/279/files

Otherwise this can be outdated and is only set once a transaction is made.

Copy link
Contributor Author

@ca11ab1e ca11ab1e Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll simply revert that change, it was out of the scope anyway. Let's tackle that in a specific PR when starknet.py will be ready (cf #80).

Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marvelous work as always!

We can merge now if in a hurry else it is better to get the gas price value from the latest block I think. I opened a PR on starknet.py because I noticed the value was available in the response

@antazoey antazoey merged commit 00300b9 into ApeWorX:main Aug 2, 2022
@ca11ab1e ca11ab1e deleted the fix-52-gas-estimation-devnet branch August 2, 2022 12:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to estimate gas cost on the devnet
2 participants