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

Add BaseApp.Seal #1812

Merged
merged 6 commits into from
Aug 1, 2018
Merged

Add BaseApp.Seal #1812

merged 6 commits into from
Aug 1, 2018

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Jul 25, 2018 •
edited by rigelrozanski
Loading

  • Linked to github-issue with discussion and accepted design
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md
  • Updated cmd/gaia and examples/

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)

Closes: #1645

@cwgoes
Copy link
Contributor

cwgoes commented Jul 25, 2018

Hmm, did we discuss this somewhere (please link)? What additional safety does this provide? Modules shouldn't have access to baseapp anyways.

@mossid mossid force-pushed the joon/1645-baseapp-seal branch from 2ed4a21 to 559b4c9 Compare July 25, 2018 00:59
mossid added a commit that referenced this pull request Jul 25, 2018
@mossid
Merge pull request #1812: Add BaseApp.Seal
559b4c9 
fix pow
@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 25, 2018 •
edited
Loading

It was discussed in #1645. This is so that things can now use the functional initial configuration method.

@mossid mossid changed the title Add BaseApp.Seal WIP: Add BaseApp.Seal Jul 25, 2018
@mossid mossid added the wip label Jul 25, 2018
ValarDragon
ValarDragon reviewed Jul 25, 2018
View reviewed changes
baseapp/baseapp.go Outdated
@@ -133,25 +136,73 @@ func (app *BaseApp) MountStore(key sdk.StoreKey, typ sdk.StoreType) {
}

// nolint - Set functions
func (app *BaseApp) SetName(name string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move these setters to a different file?

@codecov
Copy link

codecov bot commented Jul 25, 2018 •
edited
Loading

Codecov Report

Merging #1812 into develop will decrease coverage by 0.28%.
The diff coverage is 26.92%.

@@             Coverage Diff             @@
##           develop    #1812      +/-   ##
===========================================
- Coverage    63.83%   63.55%   -0.29%     
===========================================
  Files          118      119       +1     
  Lines         7024     7062      +38     
===========================================
+ Hits          4484     4488       +4     
- Misses        2285     2314      +29     
- Partials       255      260       +5

@ValarDragon
Copy link
Contributor

Should we add a enforceSeal method to baseapp, which will make sure its sealed? Then we could call this method in the other baseapp functions, just to limit problems in case a developer forgot to seal their baseapp?

Not really sure on the above, since idk how much we want to optimize for developers who forget to do things.

@cwgoes
Copy link
Contributor

cwgoes commented Jul 25, 2018

It was discussed in #1645. This is so that things can now use the functional initial configuration method.

Thanks! Makes sense.

alexanderbez
alexanderbez reviewed Jul 25, 2018
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.

So far looks great! I have this implemented in Ethermint as well, except I have #Seal is a private method which I call after all the options have been called. I suggest we do that here as well 👍

@mossid
Copy link
Contributor Author

mossid commented Jul 27, 2018

A lot of code is using Set* after NewBaseApp on their code for testing so it will hard to move it into options so we can handle settings before sealing. Maybe address it in another PR? @alexanderbez

@ValarDragon
Copy link
Contributor

I think thats a good idea @alexanderbez! That will require refactoring a lot of test cases though. Because thats a refactor we can punt until later, I think we should merge this PR without that, and then make a new issue for your idea.

@mossid mossid force-pushed the joon/1645-baseapp-seal branch from 559b4c9 to 3fb3106 Compare July 27, 2018 01:23
mossid added a commit that referenced this pull request Jul 27, 2018
@mossid
Merge pull request #1812: Add BaseApp.Seal
3fb3106 
fix pow

move setter functions

apply requests
@alexanderbez
Copy link
Contributor

Ok, great! What is left to be done?

@mossid
Copy link
Contributor Author

mossid commented Jul 27, 2018 •
edited
Loading

How do you think about inserting app.Seal() in initFromStore, which actually loads state from the database so the setting must not be done after it is called. Then we can make Seal() optional.

@ValarDragon
Copy link
Contributor

Sounds good to me! Agreed that we should still keep the seal function as well

@mossid mossid changed the title WIP: Add BaseApp.Seal Add BaseApp.Seal Jul 28, 2018
mossid added 3 commits July 31, 2018 14:31
@mossid mossid force-pushed the joon/1645-baseapp-seal branch from ddddde1 to d5a444d Compare July 31, 2018 21:32
ValarDragon
ValarDragon approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@ValarDragon
Copy link
Contributor

Can you fix the failing test?

@mossid mossid force-pushed the joon/1645-baseapp-seal branch from 0b26c98 to 9d21332 Compare July 31, 2018 22:10
rigelrozanski
rigelrozanski approved these changes Aug 1, 2018
View reviewed changes
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.

good work, I like the new setters.go file - good separation

@rigelrozanski rigelrozanski merged commit 7b54e4b into develop Aug 1, 2018
@rigelrozanski rigelrozanski deleted the joon/1645-baseapp-seal branch August 1, 2018 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@rigelrozanski rigelrozanski rigelrozanski approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

Assignees

@cwgoes cwgoes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mossid @cwgoes @ValarDragon @alexanderbez @rigelrozanski