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

R4R: Add Fee & Gas Spec #3241

Merged
merged 14 commits into from
Jan 24, 2019
Merged

R4R: Add Fee & Gas Spec #3241

merged 14 commits into from
Jan 24, 2019

Conversation

zmanian
Copy link
Member

@zmanian zmanian commented Jan 6, 2019

Spec for minimum fee spec

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)

@zmanian zmanian requested a review from zramsay as a code owner January 6, 2019 03:50
alexanderbez
alexanderbez reviewed Jan 6, 2019
View reviewed changes
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.

Thanks for starting this @zmanian -- I left a few comments 👍

docs/spec/auth/README.md Outdated Show resolved Hide resolved
docs/spec/auth/min_fee.md Outdated Show resolved Hide resolved
docs/spec/auth/min_fee.md Outdated Show resolved Hide resolved
docs/spec/auth/min_fee.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 6, 2019 •
edited
Loading

Codecov Report

Merging #3241 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3241      +/-   ##
===========================================
- Coverage    54.88%   54.87%   -0.01%     
===========================================
  Files          133      133              
  Lines         9555     9556       +1     
===========================================
  Hits          5244     5244              
- Misses        3989     3990       +1     
  Partials       322      322

@zmanian
Copy link
Member Author

zmanian commented Jan 7, 2019

Intended to close #3115

@alexanderbez alexanderbez changed the title Added Minimum fee spec R4R: Added Minimum fee spec Jan 7, 2019
@alexanderbez alexanderbez added T:Docs Changes and features related to documentation. spec ready-for-review labels Jan 7, 2019
alexanderbez
alexanderbez suggested changes Jan 7, 2019
View reviewed changes
docs/spec/auth/min_fee.md Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Some more minor fixes @zmanian 👍

@gamarin2
Copy link
Contributor

gamarin2 commented Jan 8, 2019

Needs to be updated after #3258

@alexanderbez
Copy link
Contributor

Actually, I think we may be able to close this as I plan on handling docs in #3258. Opinions @zmanian @gamarin2 ?

@zmanian
Copy link
Member Author

zmanian commented Jan 10, 2019

I think there should be a spec document in addition to usage documentation that provides help on the conceptual framework for fees.

@alexanderbez
Copy link
Contributor

Actually, yes, you're completely right @zmanian -- I'll focus on examples/usage.

@cwgoes
Copy link
Contributor

cwgoes commented Jan 11, 2019

Needs to be updated for #3258 as we will have minimum gas prices instead of minimum flat fees.

@alexanderbez
Copy link
Contributor

I updated the docs -- should be ready to review/merge.

@alexanderbez alexanderbez changed the title R4R: Added Minimum fee spec R4R: Add Fee & Gas Spec Jan 11, 2019
cwgoes
cwgoes previously requested changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Wording, and can we include instructions on how to set minimum fees in config.toml (if possible)?

docs/spec/auth/gas_fees.md Outdated Show resolved Hide resolved
docs/spec/auth/gas_fees.md Outdated Show resolved Hide resolved
docs/spec/auth/gas_fees.md Outdated Show resolved Hide resolved
docs/spec/auth/gas_fees.md Outdated Show resolved Hide resolved
docs/spec/auth/gas_fees.md Outdated Show resolved Hide resolved
@cwgoes
Copy link
Contributor

cwgoes commented Jan 22, 2019

can we include instructions on how to set minimum fees in config.toml (if possible)

cc @jackzampolin

@alessio
Copy link
Contributor

alessio commented Jan 23, 2019

--minimum-gas_prices should be --minimum-gas-prices - though this is not on @zmanian - let's change the flag to --minimum-gasprices, shall we?

@jackzampolin
Copy link
Member

@alessio I like --min-gas-prices tbh...

@alessio
Copy link
Contributor

alessio commented Jan 23, 2019 via email

@alexanderbez
Copy link
Contributor

alexanderbez commented Jan 23, 2019 •
edited
Loading

The thing is, it has to be snake case because of viper and config toml template. Im ok with --min_gasprices. If you know of a way to allow dash-cased, please lmk!

alessio pushed a commit that referenced this pull request Jan 23, 2019
Use dash-separated flags for minimus gas prices
b4f3a99 
Ref #3241
@alessio alessio mentioned this pull request Jan 23, 2019
Merged
5 tasks
@alessio
Copy link
Contributor

alessio commented Jan 23, 2019

#3364

Docs update should be carried out in the context of this PR I reckon

@alexanderbez
Copy link
Contributor

Yes -- thanks @alessio

melekes
melekes approved these changes Jan 24, 2019
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍰

@jackzampolin jackzampolin dismissed cwgoes’s stale review January 24, 2019 16:37

comments addressed

@jackzampolin jackzampolin merged commit f5e6685 into develop Jan 24, 2019
@alexanderbez alexanderbez deleted the zaki/min_fee_spec branch February 9, 2019 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin left review comments

@alexanderbez alexanderbez alexanderbez approved these changes

@melekes melekes melekes approved these changes

@alessio alessio alessio approved these changes

@cwgoes cwgoes cwgoes left review comments

@zramsay zramsay Awaiting requested review from zramsay

@ValarDragon ValarDragon Awaiting requested review from ValarDragon

Assignees
No one assigned
Labels
spec T:Docs Changes and features related to documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zmanian @alexanderbez @gamarin2 @cwgoes @alessio @jackzampolin @melekes