Chore: remove event emission on channel upgrade #6063
Conversation
hoangdv2429
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThe recent updates aim to optimize event emission during channel upgrades by selectively including essential attributes. This refinement involves removing specific fields from event emissions in certain functions and adjusting how other fields are handled. The goal is to emit only necessary upgrade fields at key points, aligning with the objectives outlined during an internal security audit regarding channel upgradability. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- modules/core/04-channel/keeper/events.go (6 hunks)
Additional comments: 19
modules/core/04-channel/keeper/events.go (19)
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-19]
The
emitChannelOpenInitEventfunction emits a channel open init event with attributes includingConnectionHopsandVersion. Given the PR's objective to reduce unnecessary event emissions, consider if the inclusion of these attributes aligns with the optimization goals. If these attributes are not critical for the event's purpose, removing them could streamline the event emission process.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-29]
The
emitChannelOpenTryEventfunction similarly includesConnectionHopsandVersionattributes. As with the previous function, evaluate the necessity of these attributes in the context of the PR's optimization objectives. Removing non-critical attributes can contribute to the efficiency of event emissions during channel upgrades.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [32-38]
In the
emitChannelOpenAckEventfunction, theConnectionHopsattribute is included. Given the PR's focus on optimizing event emissions, consider whether this attribute is essential for the event's purpose. If it is not critical, removing it could further align with the PR's objectives.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-47]
The
emitChannelOpenConfirmEventfunction also emits an event with theConnectionHopsattribute. Review the necessity of this attribute in light of the PR's optimization goals. If the attribute is not required for the event's intended use, consider removing it to enhance event emission efficiency.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [50-56]
The
emitChannelCloseInitEventfunction includes theConnectionHopsattribute in its event emission. Assess the importance of this attribute in the context of optimizing event emissions. If it is deemed non-essential, removing it could contribute to the PR's objectives.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [59-65]
In the
emitChannelCloseConfirmEventfunction, the inclusion of theConnectionHopsattribute is observed. Evaluate its necessity considering the PR's aim to optimize event emissions. If removing it aligns with the optimization goals, consider doing so to streamline the event emission process.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [68-83]
The
emitSendPacketEventandemitRecvPacketEventfunctions include deprecated attributes related to packet data and connection information. Given the PR's focus on optimizing event emissions, review the continued inclusion of these deprecated attributes. Removing or updating them could enhance the efficiency and clarity of event emissions.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [86-101]
In the
emitWriteAcknowledgementEventandemitAcknowledgePacketEventfunctions, deprecated attributes are used. Consider the relevance of these attributes in the context of the PR's optimization objectives. If they are not essential or are outdated, removing or updating them could contribute to more efficient event emissions.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [104-115]
The
emitTimeoutPacketEventfunction includes theConnectionIDattribute in its event emission. Assess the necessity of this attribute given the PR's aim to optimize event emissions. If it is not critical for the event's purpose, consider removing it to align with the optimization goals.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [118-128]
In the
emitChannelClosedEventfunction, theConnectionIDattribute is included. Evaluate its importance in the context of optimizing event emissions. If removing it contributes to the PR's objectives, consider doing so to enhance the efficiency of event emissions.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [131-142]
The
EmitChannelUpgradeInitEventfunction focuses on emitting a channel upgrade init event with theUpgradeSequenceattribute. This change aligns with the PR's objective to streamline event emissions by focusing on essential attributes. Ensure that the removal of other attributes (e.g.,ConnectionHops,Version) from this and similar functions is intentional and aligns with the optimization goals.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [145-156]
The
EmitChannelUpgradeTryEventfunction emits a channel upgrade try event, similarly focusing on theUpgradeSequenceattribute. This approach is consistent with the PR's objective to optimize event emissions. Confirm that the exclusion of non-essential attributes is deliberate and contributes to the efficiency of the upgrade process.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [159-170]
In the
EmitChannelUpgradeAckEventfunction, the event emission is optimized by including only theUpgradeSequenceattribute. This change is in line with the PR's goals. Verify that the removal of other attributes not directly related to the upgrade sequence is intentional and beneficial for the optimization of event emissions.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [173-184]
The
EmitChannelUpgradeConfirmEventfunction emits an event with a focus on theUpgradeSequenceattribute, aligning with the PR's optimization objectives. Ensure that the decision to exclude other attributes is intentional and contributes to streamlining the event emission process during channel upgrades.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [187-198]
The
EmitChannelUpgradeOpenEventfunction emits an event centered on theUpgradeSequenceattribute, consistent with the PR's aim to optimize event emissions. Confirm that the exclusion of additional attributes is deliberate and enhances the efficiency of the event emission process.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [201-214]
The
EmitChannelUpgradeTimeoutEventfunction emits an upgrade timeout event with a focus on theUpgradeSequenceand timeout-related attributes. This change appears to align with the PR's objectives by emphasizing essential attributes for the event's purpose. Ensure that the inclusion of timeout attributes is necessary and contributes to the optimization of event emissions.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [217-229]
In the
EmitErrorReceiptEventfunction, the event emission includes theUpgradeSequenceattribute and an error message. This approach aligns with the PR's optimization goals by focusing on essential information. Confirm that the inclusion of the error message is necessary and enhances the clarity and usefulness of the event emission.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [232-243]
The
EmitChannelUpgradeCancelEventfunction emits an upgraded cancelled event with a focus on theUpgradeSequenceattribute. This change is consistent with the PR's objective to optimize event emissions. Verify that the exclusion of other attributes is intentional and contributes to the efficiency of the event emission process.
- 280-285: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [246-257]
The
emitChannelFlushCompleteEventfunction emits a flushing event with a focus on the channel state. This function does not directly relate to the PR's objective of optimizing event emissions during channel upgrades but is included for completeness. Ensure that the inclusion of the channel state attribute aligns with the overall event emission strategy and contributes to the clarity of the event.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/core/04-channel/keeper/events.go (6 hunks)
Additional comments: 2
modules/core/04-channel/keeper/events.go (2)
- 373-377: The changes in
EmitChannelUpgradeTimeoutEventcorrectly adjust the attributes being emitted to focus on essential details for the upgrade process. Ensure that thecurrentChannelvariable is correctly defined and accessible within this function's scope.- 406-414: The changes in
EmitChannelUpgradeCancelEventcorrectly adjust the attributes being emitted to focus on essential details for the upgrade process. Ensure that thecurrentChannelvariable is correctly defined and accessible within this function's scope.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- modules/core/04-channel/keeper/events.go (6 hunks)
- modules/core/keeper/msg_server_test.go (15 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/04-channel/keeper/events.go
Additional comments: 1
modules/core/keeper/msg_server_test.go (1)
- 932-937: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1065]
The tests cover a wide range of scenarios for packet handling, client recovery, packet acknowledgment, packet timeout, and channel upgrade processes. The use of
pathsetup and teardown for each test case ensures isolation and repeatability. The assertions are well-placed to check both the success and failure conditions appropriately.
hoangdv2429
changed the title
|
Looks like some channel upgrades e2e tests are consistently failing, so could be that we have removed too much. 😅 I will take a look. |
no problem sir, I'll run it locally and fix it 🙏 |
hoangdv2429
requested review from
crodriguezvega,
charleenfei and
DimitrisJim
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- e2e/testsuite/testconfig.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
e2e/testsuite/testconfig.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (1)
e2e/testsuite/testconfig.go (1)
65-65: Ensure the new Hermes tag"sean-channel-upgradability"is compatible with the current test suite and update any relevant documentation to reflect this change.Verification successful
The verification process confirms that the new Hermes tag
"sean-channel-upgradability"is only referenced ine2e/testsuite/testconfig.goand does not appear in any other configuration or documentation files within the codebase. This suggests that the change is isolated. It remains important to ensure that this tag is compatible with the current test suite and to update any relevant documentation accordingly. No further issues were found through this verification process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the new Hermes tag is used in any other configuration or documentation files. rg "sean-channel-upgradability" --files-with-matchesLength of output: 80
|
A new hermes tag fixed the e2e tests. Thank you, @ljoss17! |
| @@ -62,7 +62,7 @@ const ( | |||
| // TODO: https://github.com/cosmos/ibc-go/issues/4965 | |||
| defaultHyperspaceTag = "20231122v39" | |||
| // defaultHermesTag is the tag that will be used if no relayer tag is specified for hermes. | |||
| defaultHermesTag = "luca_joss-channel-upgrade-authority" | |||
| defaultHermesTag = "sean-channel-upgradability" | |||
There was a problem hiding this comment.
have we been using this tag for the last few months?
There was a problem hiding this comment.
We were using luca_joss-..., but when these attributes were removed, Luca discovered while debugging that there was a bug in hermes and fixed it in this new tag.
There was a problem hiding this comment.
For more context, the branch sean-channel-upgradability was the branch for the channel upgrade beta implementation and the luca_joss-channel-upgrade-authority was the branch adding the changes that came after the beta (e.g. initiating an upgrade only with authority). Between the release of channel upgrade and now the branch luca_joss-channel-upgrade-authority has been merged in sean-channel-upgradability so that we only have one open PR related to channel upgrade. New Hermes Docker images for channel upgrade, until it is merged, will be in the tag sean-channel-upgradability.
Description
closes: #5741
This PR will remove emitting the full upgrade on every handler.
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit