Regen network/skip upgrade #5367

anilcse · 2019-12-05T17:17:44Z

Closes: regen-network#5

Description

For contributor use:

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/upgrade/internal/keeper/utils.go

server/start.go

x/upgrade/abci.go

x/upgrade/abci_test.go

x/upgrade/internal/keeper/utils.go

x/upgrade/internal/types/proposal.go

x/upgrade/module.go

server/start.go

x/upgrade/abci.go

x/upgrade/abci_test.go

server/start.go

x/upgrade/internal/keeper/keeper.go

x/upgrade/module.go

sahith-narahari · 2019-12-09T18:11:21Z

Made requested changes except workaround on viper, will update once I change how the array of heights is being read

anilcse · 2019-12-21T15:24:38Z

@alexanderbez @fedekunze all the changes are updated. It's ready for review.

x/upgrade/abci_test.go

alexanderbez

Looking good @anilcse -- left a few more comments 👍

Question: skipUpgradeHeights are provided to the module from user CLI input, no? This array/map dictates the control flow of BeginBlock (in that it clears the plan). What if different nodes set (or dont set at all) different values of skipUpgradeHeights? Is this OK?

x/upgrade/alias.go

x/upgrade/internal/keeper/keeper.go

x/upgrade/internal/types/utils.go

x/upgrade/abci.go

alexanderbez · 2019-12-24T14:22:54Z

simapp/app.go

@@ -131,7 +131,7 @@ type SimApp struct {

 // NewSimApp returns a reference to an initialized SimApp.
 func NewSimApp(


TBH, I'm not totally thrilled about updating the app's constructor this way simply to proxy the argument to the upgrade module's constructor. But I cannot think of a cleaner way of doing it :-/

Thoughts @aaronc @ethanfrey?

Not blocking, but I think we can think of cleaner way by passing in keeper "option" functions similar to the way we handle baseapp. However, this requires more plumbing and most notably requires the semantics of keepers to chance by operating on references and not copies.

I like your approach @alexanderbez

x/upgrade/abci.go

Fix review suggestions

anilcse · 2019-12-25T09:25:54Z

@alexanderbez thanks for the quick review. Updated all the suggestions.

alexanderbez

ACK -- left two more (non-blocking) comments. Thanks @anilcse 🎉

@fedekunze please re-review at your earliest convenience.

alexanderbez · 2019-12-26T19:36:55Z

x/upgrade/abci_test.go

  Name: test
  Height: 100
  Info: `, upgrade.Plan{Name: "test", Height: 100}.String())
 }

-func TestTestSuite(t *testing.T) {
-	suite.Run(t, new(TestSuite))
+func VerifyNotDone(t *testing.T, newCtx sdk.Context, name string) {


nit/non-blocking: the structure & style of tests and comments can definitely be cleaned up. e.g.

space between // and start comment

avoid punctuation unless it's a sentence

avoid group var statements when you assign values

remove commented-out code

Noted. We'll ensure these things are taken care from next time. Any specific reason to avoid group var statements?

var ( foo int64 = 4 bar bool = true )

Instead,

foo := 4 // or foo := int64(4) bar := true

I understand. Just wanted to know if there's any specific reason to avoid group declarations. I think having group declartion for this particular scenario is needed as it is making more readable. It is creating a bit of inconsistency in the code, too. We can take a call.

alexanderbez · 2019-12-26T19:39:27Z

simapp/app.go

@@ -131,7 +131,7 @@ type SimApp struct {

 // NewSimApp returns a reference to an initialized SimApp.
 func NewSimApp(


Not blocking, but I think we can think of cleaner way by passing in keeper "option" functions similar to the way we handle baseapp. However, this requires more plumbing and most notably requires the semantics of keepers to chance by operating on references and not copies.

x/upgrade/abci.go

…ork/skip-upgrade # Conflicts: # x/upgrade/abci_test.go # x/upgrade/internal/keeper/keeper.go

fedekunze · 2019-12-30T14:49:50Z

simapp/app.go

@@ -131,7 +131,7 @@ type SimApp struct {

 // NewSimApp returns a reference to an initialized SimApp.
 func NewSimApp(


I like your approach @alexanderbez

fedekunze · 2019-12-30T14:51:41Z

x/upgrade/abci_test.go

 	module  module.AppModule
 	keeper  upgrade.Keeper
 	querier sdk.Querier
 	handler gov.Handler
 	ctx     sdk.Context
 }

-func (s *TestSuite) SetupTest() {


why did this change? It was cleaner before imo

We were using viper previously and after your review we understood viper should be sticked to cli and it should not be used inside keeper. The change in code needed a way to set different skipUpgradeHeights everytime, so we had to replace the Suite with this setupTest to expect skipUpgradeHeights as a param

x/upgrade/internal/keeper/keeper.go

fedekunze

utACK

codecov · 2020-01-02T18:26:17Z

Codecov Report

Merging #5367 into master will increase coverage by 0.13%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #5367      +/-   ##
==========================================
+ Coverage   54.38%   54.52%   +0.13%     
==========================================
  Files         313      318       +5     
  Lines       18836    19137     +301     
==========================================
+ Hits        10244    10434     +190     
- Misses       7807     7917     +110     
- Partials      785      786       +1

Impacted Files	Coverage Δ
simapp/test_helpers.go	`0% <0%> (ø)`	⬆️
x/upgrade/abci.go	`100% <100%> (ø)`	⬆️
simapp/app.go	`89.63% <100%> (+0.12%)`	⬆️
x/upgrade/handler.go	`80% <0%> (-5.72%)`	⬇️
x/params/proposal_handler.go	`73.68% <0%> (-4.1%)`	⬇️
x/mock/test_utils.go	`60% <0%> (-3.05%)`	⬇️
x/auth/types/stdtx.go	`58.67% <0%> (-2.87%)`	⬇️
types/errors/errors.go	`67.16% <0%> (-1.59%)`	⬇️
x/mock/app.go	`62.83% <0%> (-1.36%)`	⬇️
x/bank/internal/keeper/keeper.go	`67.85% <0%> (-0.82%)`	⬇️
... and 59 more

anilcse requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners December 5, 2019 17:17

fedekunze added the C:x/upgrade label Dec 6, 2019

sahith-narahari force-pushed the regen-network/skip-upgrade branch from 395c40e to 324bf3c Compare December 6, 2019 13:37

golangcibot reviewed Dec 6, 2019

View reviewed changes

x/upgrade/internal/keeper/utils.go Outdated Show resolved Hide resolved

fedekunze suggested changes Dec 6, 2019

View reviewed changes

alexanderbez added the R4R label Dec 9, 2019

alexanderbez suggested changes Dec 9, 2019

View reviewed changes

Sahith Reddy Narahari and others added 18 commits December 21, 2019 14:11

restructured beginblocker

ee7fc82

added skip upgrade flag

ab7b501

added skip upgrade flag to server

0c37933

Added skip upgrade flag

ff0dc8f

Relocated viper to BeginBlock

a739037

Modified BeginBlocker

685eb19

Use viper.GetBool instead of cast

9542709

Added test scripts for skip upgrade

b6c64bf

removed redundant statements

891bba8

Added verify upgrade

ef5d1d1

Added verify set

ccabf1f

Replaced bool with upgrade height

4e46b29

Rennamed unsafe skip upgrade to skip upgrade

255facf

Resolved inconsistency with names

9176e43

Fixed type conversion for flag skip upgrade

dae1e51

Modified skip upgrade flag to accept multiple values

cd76b8b

Refactored test scripts

c629e4b

Refactored code in module

e812eff

Remove flag usage from internal

ab06ecb

anilcse requested review from fedekunze and alexanderbez December 21, 2019 15:24

golangcibot reviewed Dec 21, 2019

View reviewed changes

x/upgrade/abci_test.go Outdated Show resolved Hide resolved

anilcse added 2 commits December 21, 2019 23:13

Fix gofmt

abf4f80

Add more tests to utils

97070bc

alexanderbez suggested changes Dec 24, 2019

View reviewed changes

anilcse added 5 commits December 25, 2019 12:09

Fix review suggestions

9f7b9e3

Replace all the skip-upgrade usage from array to map

edb67d4

Improve godoc

ceddad5

Fix skipupgrade tests

40dc4f6

Merge pull request #12 from regen-network/skip-upgrade-review-fixes

855f474

Fix review suggestions

anilcse requested a review from alexanderbez December 25, 2019 09:26

alexanderbez approved these changes Dec 26, 2019

View reviewed changes

alexanderbez and others added 4 commits December 26, 2019 14:41

Merge branch 'master' into regen-network/skip-upgrade

0e06989

Update godoc

711db9f

Refactor comments

72a0a5b

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

4f7cdfc

…ork/skip-upgrade # Conflicts: # x/upgrade/abci_test.go # x/upgrade/internal/keeper/keeper.go

fedekunze suggested changes Dec 30, 2019

View reviewed changes

anilcse and others added 3 commits January 2, 2020 20:38

Replace returning mutable map with copy instead

adc95cd

Refactor check skip upgrade height

117b2b2

Merge branch 'master' into regen-network/skip-upgrade

a2bf9c3

fedekunze approved these changes Jan 2, 2020

View reviewed changes

Merge branch 'master' into regen-network/skip-upgrade

d664777

alexanderbez merged commit a5fc7d6 into cosmos:master Jan 3, 2020

ryanchristo deleted the regen-network/skip-upgrade branch December 12, 2022 18:14

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5367: Regen network/skip upgrade

a5071f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regen network/skip upgrade #5367

Regen network/skip upgrade #5367

anilcse commented Dec 5, 2019 •

edited

Loading

sahith-narahari commented Dec 9, 2019

anilcse commented Dec 21, 2019

alexanderbez left a comment •

edited

Loading

alexanderbez Dec 24, 2019

alexanderbez Dec 26, 2019

fedekunze Dec 30, 2019

anilcse commented Dec 25, 2019

alexanderbez left a comment •

edited

Loading

alexanderbez Dec 26, 2019

anilcse Dec 26, 2019

alexanderbez Dec 27, 2019

anilcse Dec 28, 2019

alexanderbez Dec 26, 2019

fedekunze Dec 30, 2019

fedekunze Dec 30, 2019 •

edited

Loading

anilcse Jan 2, 2020

fedekunze left a comment

codecov bot commented Jan 2, 2020

		@@ -131,7 +131,7 @@ type SimApp struct {

		// NewSimApp returns a reference to an initialized SimApp.
		func NewSimApp(

Regen network/skip upgrade #5367

Regen network/skip upgrade #5367

Conversation

anilcse commented Dec 5, 2019 • edited Loading

Description

sahith-narahari commented Dec 9, 2019

anilcse commented Dec 21, 2019

alexanderbez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilcse commented Dec 25, 2019

alexanderbez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2020

Codecov Report

anilcse commented Dec 5, 2019 •

edited

Loading

alexanderbez left a comment •

edited

Loading

alexanderbez left a comment •

edited

Loading

fedekunze Dec 30, 2019 •

edited

Loading