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

Emulator Feature: Report gas used #87

Merged
merged 7 commits into from
Sep 29, 2021

Conversation

avcdsld
Copy link
Contributor

@avcdsld avcdsld commented Sep 26, 2021

Closes: FLIP Fest issue 12 onflow/flip-fest#12

Description

There should be a way to see transaction gas (computation) usage.

With this change, the output result of the transaction execution will look like this

BEFORE
スクリーンショット 2021-09-27 0 02 12

AFTER
スクリーンショット 2021-09-29 9 06 58

Other

Since there was no specific test code for the standard output of "txID=xxxxxx" that is currently displayed, I decided that there was no need to add any specific test code for this change as well.

I checked to see if any changes were needed in this document, but it looks like no changes are needed.
https://docs.onflow.org/flow-cli/start-emulator/


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@sideninja
Copy link
Contributor

Thank you for adding this, it looks good, however, it does highlight the lack of tests for convert/vm.go and types/result.go so if you could please add those it would be great.

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Missing tests needs to be implemented

@avcdsld
Copy link
Contributor Author

avcdsld commented Sep 28, 2021

Thanks for checking the code.
I've added tests for convert/vm.go and types/result.go. I hope you can check it.

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks great, just added some minor comments about naming.

convert/vm.go Outdated Show resolved Hide resolved
convert/vm_test.go Outdated Show resolved Hide resolved
server/backend/backend.go Outdated Show resolved Hide resolved
server/backend/backend.go Outdated Show resolved Hide resolved
@avcdsld
Copy link
Contributor Author

avcdsld commented Sep 29, 2021

Looks great, just added some minor comments about naming.

I see, I also think that if it is more appropriate to use that term than Gas, it should be modified.
Thanks for the confirmation and the suggestion!

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