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.

Sign up for GitHub

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

Jump to bottom

Change --gas=simulate to --gas=auto when sending txs #3162

Closed
3 of 4 tasks
alessio opened this issue Dec 19, 2018 · 7 comments
Closed
3 of 4 tasks

Change --gas=simulate to --gas=auto when sending txs #3162

alessio opened this issue Dec 19, 2018 · 7 comments
Assignees
@alessio
Labels
C:CLI S:proposed T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: UX

Comments

@alessio
Copy link
Contributor

alessio commented Dec 19, 2018 •
edited
Loading

Users may be confused by the flag --simulate and the simulate value of the --gas flag.

--gas=simulate sets the gas to estimated value returned by the simulation that runs before the tx is executed. Conversely, --simulate just triggers the simulation without eventually execution the transaction.

Hereby I propose to change the simulate value accepted by the --gas flag to auto.

@fedekunze

For Admin Use

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

fedekunze commented Dec 19, 2018 •
edited
Loading

I'm all for gas=auto. I was actually confused bc I thought it was a duplicate from the simulate flag

@fedekunze
Copy link
Collaborator

fedekunze commented Dec 19, 2018 •
edited
Loading

also on the TxBuilder, the SimulateGas field is confusing, should say instead SimulateAndExecute or something like that

@fedekunze
Copy link
Collaborator

The NewTxBuilderFromCLI() function is inconsistent since it uses the --simulate flag which should only trigger the simulation and not the execution

@alessio
Copy link
Contributor Author

alessio commented Dec 19, 2018

I think EstimateGas sounds better

@fedekunze
Copy link
Collaborator

I think EstimateGas sounds better

maybe, but my impression is that it's just estimating the gas for the simulation and not for the execution afterwards

@alessio
Copy link
Contributor Author

alessio commented Dec 19, 2018

SimulateInOrderToEstimateGasAndExecute would be more accurate indeed :D

@alessio
Copy link
Contributor Author

alessio commented Dec 19, 2018

Just kidding :) - I'd rather stress that gas will be automatically calculated

@alessio alessio mentioned this issue Dec 19, 2018
Merged
5 tasks
@alessio alessio self-assigned this Dec 19, 2018
alessio pushed a commit that referenced this issue Dec 20, 2018
change --gas=simulate to --gas=auto when sending txs
13bb9fa 
--gas=simulate caused confusion and could be
mistaken for the --simulate flag.

Closes: #3162
alessio pushed a commit that referenced this issue Dec 20, 2018
change --gas=simulate to --gas=auto when sending txs
82549ac 
--gas=simulate caused confusion and could be
mistaken for the --simulate flag.

Closes: #3162

GasFlagSimulate -> GasFlagAuto
alessio pushed a commit that referenced this issue Dec 20, 2018
change --gas=simulate to --gas=auto when sending txs
913d8a8 
--gas=simulate caused confusion and could be
mistaken for the --simulate flag.

Closes: #3162
@alessio alessio added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Dec 20, 2018
@alessio alessio mentioned this issue Jan 12, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alessio alessio

Labels
C:CLI S:proposed T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: UX
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alessio @fedekunze