-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: remove support for time based upgrades #8849
x/upgrade: remove support for time based upgrades #8849
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8849 +/- ##
==========================================
- Coverage 59.15% 59.15% -0.01%
==========================================
Files 570 570
Lines 31775 31770 -5
==========================================
- Hits 18796 18793 -3
+ Misses 10777 10776 -1
+ Partials 2202 2201 -1
|
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.
lgtm, pending changelog
we can have a governance vote to approve this upgrade at some future block time | ||
or block height (e.g. 200000). This is known as an upgrade.Plan. The v0.38.0 code will not know of this | ||
handler, but will continue to run until block 200000, when the plan kicks in at BeginBlock. It will check | ||
we can have a governance vote to approve this upgrade at some future block height (e.g. 200000). |
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.
thanks for thinking about this file and the SPEC 👍
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
…hnicallyty/cosmos-sdk into ty-8801-remove_time_upgrades
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.
Preapproving
@@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* [\#8363](https://github.com/cosmos/cosmos-sdk/pull/8363) Addresses no longer have a fixed 20-byte length. From the SDK modules' point of view, any 1-255 bytes-long byte array is a valid address. | |||
* [\#8346](https://github.com/cosmos/cosmos-sdk/pull/8346) All CLI `tx` commands generate ServiceMsgs by default. Graceful Amino support has been added to ServiceMsgs to support signing legacy Msgs. | |||
* (crypto/ed25519) [\#8690] Adopt zip1215 ed2559 verification rules. | |||
* [\#8849](https://github.com/cosmos/cosmos-sdk/pull/8849) Upgrade module no longer supports time based upgrades. |
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 suspect we want to add a Deprecated
section and add this to that
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.
utACK. Would be great to have someone on Alessio's team to approve before putting automerge on.
@sahith-narahari @jgimeno please take a look when you spare a few minutes 🙏 |
@AmauryM should we bump the version of upgrade.proto file in this PR? |
Not a buf bug. See https://docs.buf.build/breaking-rules#field_no_delete_unless_number_reserved and https://docs.buf.build/breaking-rules#field_no_delete_unless_name_reserved. We would need to change to config to allow this |
Ah yeah, okay. @technicallyty could you add those buf configs in this PR so that it's done for the future? We already merged 2-3 PRs before this one where we also introduced a |
Yes, let's please add. "Protobuf / breakage" is not required in CI currently but I am going to enable it now (/cc @alessio). |
Added an |
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=c6af0ed87ede190932f76e49a7083ba28b415d25 |
@@ -26,6 +26,8 @@ lint: | |||
breaking: | |||
use: | |||
- FILE | |||
except: | |||
- FIELD_NO_DELETE |
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 would be great if we had some comments about this line indicating that this still requires reserving the name and number. Could you add quickly in a separate PR @technicallyty ?
Description
Removes support for time based upgrades. Motivation.
closes: #8801
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