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

Issue-#1391: Add generic estimateGas() function #1394

Merged
merged 24 commits into from
May 16, 2022

Conversation

miquelcabot
Copy link
Contributor

Fixes #1391

Changes proposed in this PR:

  • Add generic estimateGas() function
  • Call this function from all the estimate gas specific functions
  • Call this function from all the functions that need to estimate the gas

@miquelcabot miquelcabot self-assigned this Apr 5, 2022
@miquelcabot miquelcabot marked this pull request as ready for review April 6, 2022 12:43
* @param {...any[]} args arguments of the function
* @return {Promise<number>} gas cost of the function
*/
export async function estimateGas(
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow write a test for this method ? I'm thinking of having picking one contract method for eg and compare the value returned by this method to the value returned by calling estimate directly on the contract method as we did before ? Or maybe something else if you have a better idea .
By doing this i think we may also not have issues with code climate coverage dropping and the diff coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bogdanfazakas I can add a Utils.test file, for example, to test functions like this one. But I also think that this function is executed in a lot of tests, because all the specific functions used to estimate gas for concrete ABI functions, like estGasCreateNFT(), estGasAddNFTTemplate()... also calls this function. And this function is also called directly from all the functions that call the smart contract functions, like createNFT(), addNFTTemplate()...

@bogdanfazakas
Copy link
Member

Looks good in general should work, do you think is worth testing it locally in the market on the estimates we have in publish flow ?

@codeclimate
Copy link

codeclimate bot commented May 12, 2022

Code Climate has analyzed commit 6f74add and detected 89 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 89

The test coverage on the diff in this pull request is 41.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 71.7% (-2.7% change).

View more on Code Climate.

@jamiehewitt15
Copy link
Member

Looks good in general should work, do you think is worth testing it locally in the market on the estimates we have in publish flow ?

Yeah, I agree it would be good to test this with the market before merging. @miquelcabot have you done that?

Also, there seems to be a lot of codeclimate issues - 89. Have you looked into fixing them?

@miquelcabot
Copy link
Contributor Author

Looks good in general should work, do you think is worth testing it locally in the market on the estimates we have in publish flow ?

Yeah, I agree it would be good to test this with the market before merging. @miquelcabot have you done that?

Also, there seems to be a lot of codeclimate issues - 89. Have you looked into fixing them?

@jamiehewitt15 Yes, I have tested locally with the market and it works.
And the codeclimate issues are related to code that I'm not modifying in this PR.

Copy link
Member

@jamiehewitt15 jamiehewitt15 left a comment

Choose a reason for hiding this comment

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

Ok, looks good to me

@miquelcabot miquelcabot merged commit 6016c66 into v4main May 16, 2022
@miquelcabot miquelcabot deleted the issue-1391-generic-estimategas-function branch May 16, 2022 11:10
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.

Create a generic estimateGas() function
3 participants