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

Move stuff from types to auth #1049

Merged
merged 12 commits into from
May 26, 2018
Merged

Move stuff from types to auth #1049

merged 12 commits into from
May 26, 2018

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented May 24, 2018

This PR looks likes there's a lot more changes than there actually are, because of having to rename the package name prefix across many many files.

The primary goal of this PR is I move the stuff that's specific to the auth anteHandler to the auth module rather than the types folder. This includes:

  • StdTx (and its related stuff i.e. StdSignDoc, etc)
  • StdFee
  • StdSignature
  • Account interface

Related to this organization, I also:

  • Got rid of AccountMapper interface (in favor of the struct already in auth module)
  • Removed the FeeHandler from the AnteHandler as discussed in process fees in endblock #1047
  • Removed GetSignatures() from Tx interface (as different Tx styles might use something different than StdSignature)
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

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.

ebuchman
ebuchman previously approved these changes May 24, 2018
Copy link
Member

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Makes a lot of sense to me. I might even suggest to have a top level dir auth and to move x/auth to auth/std. If an alternative replay-protection + signing structure comes to bear, it could be auth/non-std or whatever ...

@ebuchman
Copy link
Member

@rigelrozanski is this good to merge now re #1047 (comment) ?

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.

Still left as far as I can tell: Implement GetFees and ClearFees in auth keeper - oh and clear testing on it!

@rigelrozanski
Copy link
Contributor

@ebuchman nope - we are in agreement of the solution from the conversation but it needs to be implemented ^ see re-review comment

@sunnya97
Copy link
Member Author

@rigelrozanski Should that be added in this same PR or in a separate PR? Created an issue for it: #1058

cwgoes
cwgoes previously approved these changes May 25, 2018
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.

utACK, fees can be in this PR or not, fine either way IMO

@rigelrozanski
Copy link
Contributor

rigelrozanski commented May 25, 2018

@sunnya97 If it's in another PR we should add back the feehandler so there is a way of dealing with fees here - don't want to just remove a vital feature without replacing it. - it's simple enough to implement though so I'd opt to include the update in this PR

@jaekwon
Copy link
Contributor

jaekwon commented May 26, 2018

LGTM

@sunnya97
Copy link
Member Author

@rigelrozanski Ok, see #1064

@rigelrozanski rigelrozanski dismissed stale reviews from cwgoes and ebuchman via cb1b6ba May 26, 2018 08:28
@codecov
Copy link

codecov bot commented May 26, 2018

Codecov Report

Merging #1049 into develop will increase coverage by 0.3%.
The diff coverage is 79.41%.

@@            Coverage Diff             @@
##           develop    #1049     +/-   ##
==========================================
+ Coverage    60.58%   60.88%   +0.3%     
==========================================
  Files           79       81      +2     
  Lines         3968     4024     +56     
==========================================
+ Hits          2404     2450     +46     
- Misses        1405     1412      +7     
- Partials       159      162      +3

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.

Looking good - just updated the changelog too

@rigelrozanski rigelrozanski merged commit ae82931 into develop May 26, 2018
@rigelrozanski rigelrozanski deleted the std_to_auth branch May 26, 2018 08:43
This was referenced May 27, 2018
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
* Update DOCS_README with switch to GH Actions

* update to repo makefile

Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants