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

R4R: Change --gas=0 semantic and introduce --gas=simulate #2306

Merged
merged 6 commits into from
Sep 12, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Sep 11, 2018

Make --gas flag accept a conventional "simulate" string value in addition
to integers. Passing --gas=simulate would trigger the tx simulation and
set the gas according to the gas estimate returned by the simulation.
Any other integer value passed to --gas would be interpreted as-is and
and set as gas wanted value.

Closes: #2300

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Sep 11, 2018

Codecov Report

Merging #2306 into develop will increase coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2306      +/-   ##
===========================================
+ Coverage    63.94%   63.95%   +<.01%     
===========================================
  Files          140      140              
  Lines         8689     8688       -1     
===========================================
  Hits          5556     5556              
+ Misses        2757     2756       -1     
  Partials       376      376

Make --gas flag accept a conventional "simulate" string value in addition
to integers. Passing --gas=simulate would trigger the tx simulation and
set the gas according to the gas estimate returned by the simulation.
Any other integer value passed to --gas would be interpreted as-is and
and set as gas wanted value.

Closes: #2300
@alessio alessio force-pushed the alessio/2300-change-gas-0-semantic branch from 94ab5f9 to ceba51a Compare September 11, 2018 14:02
@alessio alessio changed the title WIP: Use custom GasSetting spf13/pflag flag to parse --gas #2300 Change --gas=0 semantic and introduce --gas=simulate Sep 11, 2018
@alessio alessio changed the title Change --gas=0 semantic and introduce --gas=simulate R4R: Change --gas=0 semantic and introduce --gas=simulate Sep 11, 2018
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments with some test cases that are missing.

Also I think we should change the gas type to uint. Auditors have rise that concern a couple times already.

client/flags.go Show resolved Hide resolved
client/lcd/lcd_test.go Outdated Show resolved Hide resolved
client/lcd/lcd_test.go Outdated Show resolved Hide resolved
cmd/gaia/cli_test/cli_test.go Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK -- left some general feedback. Also, broken lint CI needs to be addressed...not sure what is going on there.

client/flags.go Outdated Show resolved Hide resolved
client/flags.go Show resolved Hide resolved
client/flags.go Outdated Show resolved Hide resolved
@alessio
Copy link
Contributor Author

alessio commented Sep 11, 2018 via email

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

untestedACK and on latest develop 👍

@rigelrozanski rigelrozanski merged commit fb0cc0b into develop Sep 12, 2018
@rigelrozanski rigelrozanski deleted the alessio/2300-change-gas-0-semantic branch September 12, 2018 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants