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: Remove Steak Usage from Docs #3869

Closed
wants to merge 5 commits into from
Closed

R4R: Remove Steak Usage from Docs #3869

wants to merge 5 commits into from

Conversation

yangyanqing
Copy link
Contributor

@yangyanqing yangyanqing commented Mar 13, 2019

closes: #3833


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • 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)

@alexanderbez alexanderbez changed the title WIP: Fix #3833, modify stake(steak) to uatom. Remove Steak Usage from Docs Mar 13, 2019
@alexanderbez alexanderbez added T:Docs Changes and features related to documentation. WIP labels Mar 13, 2019
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.

Didn't go through this in depth on the exact numeric values, but it looks good! Thanks :)

client/flags.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #3869 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop    #3869   +/-   ##
========================================
  Coverage    60.94%   60.94%           
========================================
  Files          192      192           
  Lines        14346    14346           
========================================
  Hits          8743     8743           
  Misses        5033     5033           
  Partials       570      570

@yangyanqing
Copy link
Contributor Author

I prefer this pr to merge as soon as possible because it contains too many files and is easily conflicted with other commits. Thanks! @gamarin2 @jackzampolin

@yangyanqing yangyanqing changed the title Remove Steak Usage from Docs R4R: Remove Steak Usage from Docs Mar 13, 2019
@jackzampolin
Copy link
Member

I would like 1 more set of 👀 on this

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.

Thanks for the energy here, unfortunately the vast majority of these changes are inappropriate FTFR. Most of the changes reference SDK code which is intended to be zone independant, thus a generic invented token stake is better suited - community members have already complained about how tightly coupled the sdk and gaia - these changes are a step in the wrong direction.

The only changes that are appropriate to use uatoms instead of stake are in documentation or code specific to the gaia (aka in cmd/gaia) - a good example of this would be updating the flag descriptions within cmd/gaia to reference uatom. Most of these changes are in the sdk.

lastly, I did see that there were a couple instances of steak you found which should replaced with stake (not uatom as in this PR)

If you wanted to make this a much smaller PR that addressed the steak and gaia code only that could be useful, otherwise this PR should be closed.

client/flags.go Show resolved Hide resolved
baseapp/baseapp_test.go Outdated Show resolved Hide resolved
@zhiyuzhang86
Copy link

I prefer this pr to merge as soon as possible too, Thanks! @jackzampolin

@yangyanqing
Copy link
Contributor Author

yangyanqing commented Mar 13, 2019

Thanks for the energy here, unfortunately the vast majority of these changes are inappropriate FTFR. Most of the changes reference SDK code which is intended to be zone independant, thus a generic invented token stake is better suited - community members have already complained about how tightly coupled the sdk and gaia - these changes are a step in the wrong direction.

The only changes that are appropriate to use uatoms instead of stake are in documentation or code specific to the gaia (aka in cmd/gaia) - a good example of this would be updating the flag descriptions within cmd/gaia to reference uatom. Most of these changes are in the sdk.

lastly, I did see that there were a couple instances of steak you found which should replaced with stake (not uatom as in this PR)

If you wanted to make this a much smaller PR that addressed the steak and gaia code only that could be useful, otherwise this PR should be closed.

Thanks for your review and comment. But I think using the same denom with the gaia will be more conducive to people's understanding cosmos sdk. Moving the denom from go file to configuration file or the init command is the better way to decouple gaia and sdk.

@rigelrozanski
Copy link
Contributor

Moving the denom from go file to configuration file or the init command is the better way to decouple gaia and sdk

I think this is great idea for flag initialization, however I don't think it needs to be a part of the config, it could just be an input to the server/client commands.

Let's do that as a part of this PR to not introduce new technical debt.

This said, I still don't think the sdk testing files should having anything besides an example "stake" denom

@yangyanqing
Copy link
Contributor Author

yangyanqing commented Mar 14, 2019

Moving the denom from go file to configuration file or the init command is the better way to decouple gaia and sdk

I think this is great idea for flag initialization, however I don't think it needs to be a part of the config, it could just be an input to the server/client commands.

Let's do that as a part of this PR to not introduce new technical debt.

This said, I still don't think the sdk testing files should having anything besides an example "stake" denom

After studying the interface carefully, I agree with your opinion. Application can assign demon in app/genesis.go.

So I will close this PR and create another PR#3884 to modify typo steak only.

Thanks a lot!

@rigelrozanski

@yangyanqing yangyanqing deleted the frank/issue-3833 branch March 14, 2019 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gaiacli still uses stake denom in its docs
5 participants