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

x/upgrade: protocol version tracking #8897

Merged
merged 41 commits into from
Apr 2, 2021
Merged

Conversation

technicallyty
Copy link
Contributor

Description

  • Includes app version and protocol version fields to abci.ResponseInfo
  • x/upgrade module now tracks a protocol version which increments on upgrade in store with new key 0x3
  • x/upgrade keeper requires new argument which is an interface that implements functionality to set the protocol version on baseapp

closes: #7487


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #8897 (d86db6f) into master (410d8ed) will increase coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8897   +/-   ##
=======================================
  Coverage   58.94%   58.95%           
=======================================
  Files         575      575           
  Lines       32171    32191   +20     
=======================================
+ Hits        18962    18977   +15     
- Misses      10995    10999    +4     
- Partials     2214     2215    +1     
Impacted Files Coverage Δ
x/upgrade/types/keys.go 0.00% <ø> (ø)
x/upgrade/keeper/keeper.go 76.51% <75.00%> (-0.21%) ⬇️
baseapp/abci.go 57.94% <100.00%> (+0.19%) ⬆️
baseapp/baseapp.go 77.38% <100.00%> (+0.09%) ⬆️
baseapp/options.go 63.01% <100.00%> (-0.88%) ⬇️
simapp/app.go 82.84% <100.00%> (ø)

@orijbot
Copy link

orijbot commented Mar 16, 2021

Copy link
Contributor

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, just a small concern regarding naming.

baseapp/abci.go Outdated Show resolved Hide resolved
@orijbot
Copy link

orijbot commented Mar 18, 2021

@@ -11,6 +11,7 @@ import (
"github.com/tendermint/tendermint/libs/log"
tmos "github.com/tendermint/tendermint/libs/os"

"github.com/cosmos/cosmos-sdk/baseapp"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of importing baseapp can we duplicate the ProtocolManager interface here following the expected keepers pattern?

x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
@@ -28,16 +29,18 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryMarshaler
upgradeHandlers map[string]types.UpgradeHandler
protoManager baseapp.ProtocolManager
}

// NewKeeper constructs an upgrade Keeper
Copy link
Member

Choose a reason for hiding this comment

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

Can we document all of the parameters here? I'd also like this to explicitly allow ProtocolManager to be nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like this has been addressed @technicallyty, although I guess documenting vs ProtocolVersionSetter would be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does documentation on just the versionSetter suffice @aaronc? I can add comments to all of them, or maybe can do that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I would like comments on all of them. It can be a separate PR if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comments for all the fields @aaronc

Copy link
Member

@aaronc aaronc Mar 30, 2021

Choose a reason for hiding this comment

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

Awesome that's great. We also do want docs for all arguments to NewKeeper. That is what users of the SDK will see in godocs when they're using NewKeeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added doc comments. let me know if the format/content needs to change @aaronc

@cosmos cosmos deleted a comment from orijbot Mar 25, 2021
@orijbot
Copy link

orijbot commented Mar 26, 2021

@orijbot
Copy link

orijbot commented Mar 26, 2021

@orijbot
Copy link

orijbot commented Mar 26, 2021

@orijbot
Copy link

orijbot commented Mar 26, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@orijbot
Copy link

orijbot commented Mar 29, 2021

@clevinson clevinson added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 2, 2021
@clevinson clevinson merged commit e3a0148 into master Apr 2, 2021
@clevinson clevinson deleted the ty-7487-protocol_version branch April 2, 2021 14:11
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* -added consensus version tracking to x/upgrade

* -added interface to module manager -added e2e test for migrations using consensus version store in x/upgrade -cleaned up x/upgrade Keeper -handler in apply upgrade now handles errors and setting consensus versions -cleaned up migration map keys -removed init chainer method -simapp now implements GetConsensusVersions to assist with testing

* protocol version p1

* add protocol version to baseapp

* rebase against master

* add test

* added test case

* cleanup

* docs and change to bigendian

* changelog

* cleanup keeper

* swap appVersion and version

* cleanup

* change interface id

* update keeper field name to versionSetter

* reorder keys and docs

* -move interface into exported folder

* typo

* typo2

* docs on keeper fields

* docs for upgrade NewKeeper

* cleanup

* NewKeeper docs

Co-authored-by: technicallyty <48813565+tytech3@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BaseApp.Info should return a value for AppVersion
8 participants