-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
…ng 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
Codecov Report
@@ 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
|
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=42eae37a2970445893042777552e4b99 |
There was a problem hiding this 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.
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=d9c29c4d6a6c672c8c9e40833b9b78401483a5e7 |
x/upgrade/keeper/keeper.go
Outdated
@@ -11,6 +11,7 @@ import ( | |||
"github.com/tendermint/tendermint/libs/log" | |||
tmos "github.com/tendermint/tendermint/libs/os" | |||
|
|||
"github.com/cosmos/cosmos-sdk/baseapp" |
There was a problem hiding this comment.
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
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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-sdk into ty-7487-protocol_version
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=d20bfc10c1bdf4303db0756c886cda45f9665789 |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=b5e2e395007f4c96a5e629dc64451af5 |
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=71a4dfafcfae7ca34dc94ce2feed266dbe6da009 |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=f5af0bdbc6d34becb35658ad4b4ce5fa |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=de7b1585ec494be0810f8a9aeb448361 |
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=b0f7765e35f1fe0ea2b34ca9506355ca6453f397 |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=ed0c0a5a4b544fddb11dc71db8c9e768 |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=8b5d442aa4f246999e4480835e0b2f08 |
Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=c546d701c0004306801d65b769613fbb |
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=4bf3eaaa7b38800c1901a246057bff70aa34b275 |
* -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 <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>
Description
abci.ResponseInfo
0x3
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes