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.

Sign up for GitHub

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

Jump to bottom

Add nil checks on the controller module for underlying app #2102

Merged
merged 16 commits into from
Aug 26, 2022
Merged

Add nil checks on the controller module for underlying app #2102

merged 16 commits into from
Aug 26, 2022

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Aug 24, 2022 •
edited
Loading

Description

closes: #2040

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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@chatton chatton marked this pull request as ready for review August 24, 2022 10:02
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022 •
edited
Loading

Codecov Report

Merging #2102 (d4c5b04) into main (5de67b2) will increase coverage by 0.00%.
The diff coverage is 93.75%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2102   +/-   ##
=======================================
  Coverage   80.03%   80.03%           
=======================================
  Files         170      170           
  Lines       11813    11823   +10     
=======================================
+ Hits         9454     9463    +9     
- Misses       1941     1942    +1     
  Partials      418      418
Impacted Files Coverage Δ
...7-interchain-accounts/controller/ibc_middleware.go 85.29% <93.75%> (+0.81%) ⬆️

charleenfei
charleenfei approved these changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

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

nice one! 🎉

colin-axner
colin-axner reviewed Aug 25, 2022
View reviewed changes
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.

LGTM, left only comments on code formatting

modules/apps/27-interchain-accounts/controller/ibc_middleware.go Outdated Show resolved Hide resolved
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go Outdated Show resolved Hide resolved
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go Outdated Show resolved Hide resolved
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go Show resolved Hide resolved
modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go Show resolved Hide resolved
colin-axner
colin-axner approved these changes Aug 25, 2022
View reviewed changes
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.

Excellent! Thank you!

@colin-axner
Copy link
Contributor

Could you add a changelog entry indicating nil apps are allowed in ics27 controller middleware?

@chatton chatton enabled auto-merge (squash) August 26, 2022 08:41
@chatton chatton merged commit dcd616c into main Aug 26, 2022
@chatton chatton deleted the cian/issue#2040-add-nil-checks-on-the-controller-module-for-underlying-app branch August 26, 2022 09:30
mergify bot pushed a commit that referenced this pull request Aug 26, 2022
@chatton @mergify
Add nil checks on the controller module for underlying app (#2102)
e7b49fd 
(cherry picked from commit dcd616c)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request Aug 26, 2022
Merged
colin-axner added a commit that referenced this pull request Aug 31, 2022
@mergify @chatton @colin-axner
Add nil checks on the controller module for underlying app (backport #…
cc16ae4 
…2102) (#2131)

* Add nil checks on the controller module for underlying app (#2102)

(cherry picked from commit dcd616c)

# Conflicts:
#	CHANGELOG.md

* fix conflicts

Co-authored-by: Cian Hatton <cian@interchain.io>
Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-axner colin-axner colin-axner approved these changes

@charleenfei charleenfei charleenfei approved these changes

@seantking seantking seantking approved these changes

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal AdityaSripal is a code owner

@damiannolan damiannolan Awaiting requested review from damiannolan damiannolan is a code owner

@crodriguezvega crodriguezvega Awaiting requested review from crodriguezvega

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add nil checks on the controller module for underlying app
6 participants
@chatton @codecov-commenter @colin-axner @seantking @charleenfei @crodriguezvega