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

add default for gas-price and gas-limit (#1699) #3118

Closed

Conversation

deaswang
Copy link
Contributor

Fixes #1699

Copy link
Member

@frrist frrist 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 to me (these are the default values from the wiki), thanks!

@frrist frrist requested a review from travisperson July 26, 2019 01:07
@frrist frrist self-assigned this Jul 26, 2019
@frrist frrist added the C-ux Category: Anything related to user experience label Jul 26, 2019
@frrist frrist requested a review from anorth July 29, 2019 21:57
@frrist frrist added the RFM label Jul 29, 2019
@deaswang deaswang force-pushed the fix/1699-option-default branch from a0d0912 to 95fcc69 Compare July 30, 2019 02:28
@deaswang
Copy link
Contributor Author

already rebase master. @travisperson @frrist

@deaswang
Copy link
Contributor Author

deaswang commented Jul 30, 2019

strange, sometime pass, sometime fail at different test case here. how to retest.

@anorth
Copy link
Member

anorth commented Jul 30, 2019

The values have changed since this was reviewed. Please see my comment here: #1896 (comment)

@travisperson
Copy link
Contributor

travisperson commented Jul 30, 2019

We removed the ability for zero priced gas. (Issue #2523; PR #2605)

https://github.com/filecoin-project/go-filecoin/blob/3dbbf3a515ab9c9c48bb37da5dafc7bce77e17c0/consensus/validation.go#L67-L69

I'm not sure how these tests are passing unless the code above is dead. My guess is that we are specifying all gas prices already actually everywhere.

@anorth
Copy link
Member

anorth commented Jul 30, 2019

In that case I don't think we should have a default gas price baked into the code. Until we have something more sophisticated, I think we should require users to specify a gas price, and thus a bound on their cost. cc @mishmosh for comment.

Sorry for the misdirection from #1699, @deaswang. Can we fix up the help documentation to match behaviour instead?

@deaswang
Copy link
Contributor Author

OK. Wait for final decide. if gas price is required, it should move to Arguments. Options donot support require option.
https://godoc.org/github.com/ipfs/go-ipfs-cmdkit

@anorth
Copy link
Member

anorth commented Jul 31, 2019

In that case I think maybe we should just not fix this, and wait for #3098 and a CLI implemented on top of it to solve all our problems.

@frrist
Copy link
Member

frrist commented Aug 19, 2019

Sorry for the misdirection on this @deaswang, we are going to close this and reevaluate after #3098 lands.

@frrist frrist closed this Aug 19, 2019
@deaswang deaswang deleted the fix/1699-option-default branch August 23, 2019 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-ux Category: Anything related to user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] CLI Help when some "options" are required is self-contradicting
4 participants