-
Notifications
You must be signed in to change notification settings - Fork 586
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
chore: remove upgrade
prefix from upgrade event attributes
#5603
chore: remove upgrade
prefix from upgrade event attributes
#5603
Conversation
@@ -0,0 +1,35 @@ | |||
--- |
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 added this doc to drop a note of API breaking changes that this PR might include. I will also add later on some other notes for other API-breaking changes that have already been merged to main.
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 unbreak the event key changes in the backport! :)
AttributeKeyPortID = "port_id" | ||
AttributeKeyChannelID = "channel_id" | ||
AttributeKeyChannelState = "channel_state" | ||
AttributeVersion = "version" |
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 renamed this one to stay consistent with the format of starting with AttributeKey
.
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.
was this added in channel upgradability? Can't remember 😅 If not, wouldn't this be API breaking and as such should not be backported to 8.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.
It's already used in emitChannelOpenIntEvent
and emitChannelOpenTryEvent
, so yes, it would be API breaking. That's why I added a note to the migration docs.
AttributeKeyTimeout = "timeout" | ||
|
||
// upgrade specific keys | ||
AttributeKeyUpgradeSequence = "sequence" |
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 kept this as ...UpgradeSequence
because we already use AttributeKeySequence
below for packet_sequence
. I can renamed that one to AttributeKeyPacketSequence
is people like that. Although then I should probably rename all the packet even attribute to start with AttributeKeyPacket...
.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5603 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 197 197
Lines 15258 15258
=======================================
Hits 12394 12394
Misses 2399 2399
Partials 465 465
|
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.
minor doc fix and a another quick Q
AttributeKeyPortID = "port_id" | ||
AttributeKeyChannelID = "channel_id" | ||
AttributeKeyChannelState = "channel_state" | ||
AttributeVersion = "version" |
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.
was this added in channel upgradability? Can't remember 😅 If not, wouldn't this be API breaking and as such should not be backported to 8.1?
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
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!! 🚀
@@ -0,0 +1,35 @@ | |||
--- |
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 unbreak the event key changes in the backport! :)
…tribute-keys # Conflicts: # modules/core/04-channel/keeper/events.go # modules/core/04-channel/types/events.go # modules/core/keeper/msg_server_test.go
* chore: remove upgrade prefix from upgrade event attributes * add migration docs for v9 * Update docs/docs/05-migrations/12-v8-to-v9.md Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * rename attribute string --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> (cherry picked from commit f3c76cd)
…#5603) (#5627) * chore: remove `upgrade` prefix from upgrade event attributes (#5603) * chore: remove upgrade prefix from upgrade event attributes * add migration docs for v9 * Update docs/docs/05-migrations/12-v8-to-v9.md Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * rename attribute string --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> (cherry picked from commit f3c76cd) * chore: rm unnecessary migration doc * revert API breaking change --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
I will open a PR to hermes to do the changes there as well. I expect upgrade e2e tests might fail in the meantime.
closes: #3666
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
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.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.