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

Remove unnecessary event attribute from INIT Handshake Msgs #3867

Merged
merged 7 commits into from
Jun 30, 2023

Conversation

ruthishvitwit
Copy link
Contributor

@ruthishvitwit ruthishvitwit commented Jun 16, 2023

Description

closes: #3847

Commit Message / Changelog Entry

Removed counterparty connection/channel id from both ConnOpenInit and ChanOpenInit as they will both be empty in this stage

chore(api)!: Remove unnecessary event attribute from INIT Handshake Msgs

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.

  • 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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@ruthishvitwit ruthishvitwit changed the title chore: Removed event attribute from INIT Handshake Msgs chore: Removed unnecessary event attribute from INIT Handshake Msgs Jun 16, 2023
@DimitrisJim DimitrisJim changed the title chore: Removed unnecessary event attribute from INIT Handshake Msgs Remove unnecessary event attribute from INIT Handshake Msgs Jun 16, 2023
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, @ruthishvitwit!

From a quick look at relayer and Hermes code, it looks like it should be safe to remove these from the events.

Relevant links for relayer: this and this. If the key is not present in the event, then the parser will just not try to parse the value and it will default to empty string anyway. Besides, the e2e tests are passing in your PR, so that's also a good sign.

Relevant links for hermes: this and this. Similar thing as with relayer: if the key is not present then the value will be a None, instead of an Some(""), but that should be fine too.

Mentioning here @jtieri and @ljoss17 for extra confirmation.

Maybe it would be nice also to mention this change in the v7-to-v8 migration docs.

@charleenfei charleenfei added the type: code hygiene Clean up code but without changing functionality or interfaces label Jun 19, 2023
@ljoss17
Copy link

ljoss17 commented Jun 20, 2023

Everything looks good on Hermes side, I ran the tests and haven't seen any issues with the changes from this PR

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @ruthishvitwit! Given that e2e tests use golang relayer and they pass, it should be fine with it.

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #3867 (e55b264) into main (6fcedca) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3867      +/-   ##
==========================================
- Coverage   78.84%   78.84%   -0.01%     
==========================================
  Files         187      187              
  Lines       12950    12948       -2     
==========================================
- Hits        10211    10209       -2     
  Misses       2311     2311              
  Partials      428      428              
Impacted Files Coverage Δ
.../apps/27-interchain-accounts/host/keeper/keeper.go 87.39% <ø> (ø)
modules/core/03-connection/keeper/events.go 100.00% <ø> (ø)
modules/core/04-channel/keeper/events.go 100.00% <ø> (ø)
modules/core/04-channel/keeper/handshake.go 89.50% <ø> (ø)
.../apps/27-interchain-accounts/host/client/cli/tx.go 29.70% <100.00%> (ø)
...s/apps/27-interchain-accounts/host/keeper/relay.go 71.08% <100.00%> (ø)
modules/apps/27-interchain-accounts/types/codec.go 79.54% <100.00%> (ø)

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

thanks @ruthishvitwit 🥳

@crodriguezvega crodriguezvega merged commit 99e1d7e into cosmos:main Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code hygiene Clean up code but without changing functionality or interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INIT Handshake Msgs emit unnecessary event attribute
6 participants