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

Change gas simulation logic to a separate flag/parameter #2300

Closed
4 tasks done
fedekunze opened this issue Sep 10, 2018 · 8 comments
Closed
4 tasks done

Change gas simulation logic to a separate flag/parameter #2300

fedekunze opened this issue Sep 10, 2018 · 8 comments

Comments

@fedekunze
Copy link
Collaborator

fedekunze commented Sep 10, 2018

Summary

Change gas simulation logic to a separate flag/parameter to improve the UX

Problem Definition

As discussed with @faboweb, it's not clear from a UX perspective that you would expect to run a gas simulation by setting --gas=0. Proposed designs follow:

Proposals

Proposal A: add an extra flag

  • Add a --estimate-gas=<true/false> flag (on CLI) and parameter (on gaia-lite) to change the logic from Simulate transactions by default to set gas automatically #2047, which currently has to set --gas=0 to run the gas simulation
  • Require that the --gas flag and parameter can only take positive values
  • If both flags are given: --estimate-gas=true and --gas =$AMOUNT > 0, it runs the simulation

Proposal B: do not add an extra flag

  • Make --gas flag accept a conventional string value, e.g. simulate/auto/estimate that would trigger tx simulation and set the gas according to the gas estimate returned by the simulation.
  • Requires --gas flag values to be checked against a string conventional value/parsed into int64.

Rationale:

  • Avoid adding an extra parameter to control gas for the sake of interface's simplicity. Gas setting should be controlled via only one dedicated parameter: --gas
  • What if the user inputs both --gas=$AMOUNT and --estimate-gas? Such a command line would be misleading and ambiguous, i.e. what should the expected behaviour be when the user sets a gas allocation and switches gas simulation on? Which flag should take precedence?

Proposal C: add an extra flag and exit if conflicting flags are passed

  • Add a --estimate-gas=<true/false> flag (on CLI) and parameter (on gaia-lite) (as above)
  • Exit with an error if both --estimate-gas=true and --gas=$AMOUNT are supplied.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon
Copy link
Contributor

Once we make simulation default, I don't see why this is an issue? I'd expect setting gas to not have it be simulated (or at least only warning me if I gave an insufficient amount), and to have it be simulated by default.

Currently its not default due to it depending on the private key, which causes weird ledger UX + has sidechannel leakage concerns.

@alexanderbez
Copy link
Contributor

iff we do decide to have two flags, the command/endpoint should halt immediately and gracefully with a warning stating that both cannot be provided.

Simulation will be default now @ValarDragon ?

@alessio
Copy link
Contributor

alessio commented Sep 10, 2018

@alexanderbez I've put your suggestion up among the other proposals

@ValarDragon
Copy link
Contributor

With the newly updated proposals, Proposal B looks great to me!

@alessio
Copy link
Contributor

alessio commented Sep 10, 2018

Let's vote:

Proposal A 👍
Proposal B 😄
Proposal C 🎉

@alessio
Copy link
Contributor

alessio commented Sep 10, 2018

/cc @jackzampolin @jaekwon @cwgoes

@jackzampolin
Copy link
Member

We already have a ton of flags so adding another one will confuse matters. I voted for B because we could default to something sane (i.e. --gas=estimate) and advanced users can put in their own value for gas if they need to (i.e. --gas=100000). Seems user-friendly and understandable.

@alexanderbez
Copy link
Contributor

alexanderbez commented Sep 10, 2018

Proposal B looks sound to me. Parsing and validating can easily be done with string constants within a switch case and handling integers as the default.

alessio pushed a commit that referenced this issue 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
rigelrozanski pushed a commit that referenced this issue Sep 12, 2018
* Change --gas=0 semantic and introduce --gas=simulate

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

* Add test cases with gas=0

* ACK suggestion from @alexanderbez

* s/GasFlagSimulateString/GasFlagSimulate/

* Drop TODO comment on Gas type

* Enrich TODO with ref
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants