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 unused account in eth_estimateGas #233

Merged

Conversation

guillemsole
Copy link
Contributor

Good afternoon team, I'm using your library to build an app with web3. Thank you for your work, it has been really helpful. I would like to propose a small improvement.

While building my model I have found that EthereumClient function eth_estimateGas does not use the account parameter and would be good to remove it.

I have a model Token where I inject contract address and EthereumClient for the read functions. For the write functions I want the model to return a transaction with GasPrice and GasLimit calculated. Having the account as a parameter of eth_estimateGas, forces me to inject the account which is not used and is coupled to it.

I have maintained signature of the deprecated method.

@dioKaratzas dioKaratzas self-requested a review June 14, 2022 07:14
Copy link
Collaborator

@dioKaratzas dioKaratzas left a comment

Choose a reason for hiding this comment

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

LGTM! @DarthMike should deprecate those functions instead of removing them?

@dioKaratzas dioKaratzas requested a review from DarthMike June 14, 2022 07:39
Copy link
Member

@DarthMike DarthMike left a comment

Choose a reason for hiding this comment

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

Thanks @guillemsole

@DarthMike DarthMike merged commit 2099e64 into argentlabs:develop Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants