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

remaining review comments from #1613 #5539

Merged
merged 17 commits into from
Jan 16, 2024
Merged

Conversation

crodriguezvega
Copy link
Contributor

@crodriguezvega crodriguezvega commented Jan 8, 2024

Description

I think this PR covers the remaining comments left in the review of #1613.

The happy path e2e test for channel upgrades now also:

  • Prunes acknowledgements after the upgrade.
  • Sends an incentivised packet after the upgrade (using a multi message transaction with IBC transfer + pay packet fee). Initially I wrote the test using the async method, but the test didn't pass (packet was not relayed). I tested manually and could reproduce the same behaviour (packets are not relayed if hermes is started after submitting the packet - I have let Luca know about this behaviour). Now the test starts the relayer and then it broadcasts the multi message transaction. Thanks a lot to @chatton for the help debugging the e2e test.

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

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 the 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.

@crodriguezvega crodriguezvega added the channel-upgradability Channel upgradability feature label Jan 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d596310) 81.19% compared to head (5f6ffb4) 81.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5539      +/-   ##
==========================================
+ Coverage   81.19%   81.22%   +0.03%     
==========================================
  Files         197      197              
  Lines       15258    15258              
==========================================
+ Hits        12388    12394       +6     
+ Misses       2405     2399       -6     
  Partials      465      465              
Files Coverage Δ
modules/core/04-channel/types/msgs.go 94.80% <ø> (ø)

... and 2 files with indirect coverage changes

@crodriguezvega crodriguezvega mentioned this pull request Jan 10, 2024
9 tasks
@crodriguezvega crodriguezvega marked this pull request as ready for review January 14, 2024 20:41
@crodriguezvega crodriguezvega added the priority PRs that need prompt reviews label Jan 15, 2024
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Looks great! left some small suggestions, LGTM! 🚀

e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
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.

small typo and quick Q

e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
e2e/testsuite/grpc_query.go Show resolved Hide resolved
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.

lgtm, thanks for going through these! 💯

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks for following up on these various issues!

e2e/tests/core/04-channel/upgrades_test.go Outdated Show resolved Hide resolved
@crodriguezvega crodriguezvega merged commit 5d77221 into main Jan 16, 2024
63 of 64 checks passed
@crodriguezvega crodriguezvega deleted the carlos/review-comments-#1613 branch January 16, 2024 11:57
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* e2e test: send incentivised packet after upgrade, add extra tests for cbs

* update hermes docker image

* add prune acknowledgements to successful upgrade test

* use correct tx response

* getting further with the e2e test, addressing a couple of other review items

* refactor test to use sync incentivization instead of async

* update hermes image tag

* revert change that was breaking a test

* Apply suggestions from code review

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: DimitrisJim <[email protected]>

* rename variables for consistency

* rename variables for clarification

---------

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: DimitrisJim <[email protected]>
(cherry picked from commit 5d77221)

# Conflicts:
#	e2e/README.md
#	e2e/tests/core/04-channel/upgrades_test.go
#	e2e/tests/transfer/incentivized_test.go
#	e2e/testsuite/grpc_query.go
#	e2e/testsuite/tx.go
crodriguezvega added a commit that referenced this pull request Jan 16, 2024
* remaining review comments from #1613 (#5539)

* e2e test: send incentivised packet after upgrade, add extra tests for cbs

* update hermes docker image

* add prune acknowledgements to successful upgrade test

* use correct tx response

* getting further with the e2e test, addressing a couple of other review items

* refactor test to use sync incentivization instead of async

* update hermes image tag

* revert change that was breaking a test

* Apply suggestions from code review

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: DimitrisJim <[email protected]>

* rename variables for consistency

* rename variables for clarification

---------

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: DimitrisJim <[email protected]>
(cherry picked from commit 5d77221)

# Conflicts:
#	e2e/README.md
#	e2e/tests/core/04-channel/upgrades_test.go
#	e2e/tests/transfer/incentivized_test.go
#	e2e/testsuite/grpc_query.go
#	e2e/testsuite/tx.go

* remove e2e folder

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channel-upgradability Channel upgradability feature priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants