Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix comment for gas extern in Wasm runtime #8101

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Mar 13, 2018

No description provided.

@lexfrl lexfrl added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 13, 2018
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lexfrl lexfrl force-pushed the wasm-fix-comment-for-gas branch from 879f9b7 to 5bfd705 Compare March 13, 2018 20:43
@niklasad1 niklasad1 added the A8-looksgood 🦄 Pull request is reviewed well. label Mar 13, 2018
@debris debris removed the A0-pleasereview 🤓 Pull request needs code review. label Mar 14, 2018
@debris debris merged commit 5c47116 into master Mar 14, 2018
@debris debris deleted the wasm-fix-comment-for-gas branch March 14, 2018 11:27
@NikVolf
Copy link
Contributor

NikVolf commented Mar 14, 2018

New comment is wrong, this external is not only about block of execution, it is general gas charge; so please revert to previous or update to something with complete meaning

@lexfrl
Copy link
Contributor Author

lexfrl commented Mar 14, 2018

@NikVolf
Isn't gas_charge is about general gas charge?
Are you sure we better to revert to the previous which is:
Report gas cost with the params passed in wasm stack

@NikVolf
Copy link
Contributor

NikVolf commented Mar 14, 2018

gas_charge is not an extern function

Previous comment was at least correct. For example, grow_memory also is charged via gas extern, it's not only about blocks.

@lexfrl
Copy link
Contributor Author

lexfrl commented Mar 14, 2018

let’s just make a pull request to extend/fix the comment. The problem with the previous version was that it confuses user and not clarifies the actual purpose of the extern

@5chdn 5chdn added this to the 1.11 milestone Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants