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

fix!: queries that panic should return ack-err #86

Merged

Conversation

VitalyV1337
Copy link
Contributor

@VitalyV1337 VitalyV1337 commented Aug 14, 2023

Closes #85

@VitalyV1337 VitalyV1337 changed the title DRAFT: Queries that panic should return ack-err Queries that panic should return ack-err Aug 18, 2023
@akc2267 akc2267 requested a review from jtieri October 10, 2023 16:05
Copy link
Member

@jtieri jtieri 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 the contribution and sorry for the slow turnaround time on this one!

my one request would be to move cache_ctx.go into the keeper package since that is where code is being invoked at. then we can delete the utils package and make these functions unexported. my reasoning being that having packages named utils, helpers, common, etc. often feels like an anti-pattern and if the code is only being called in one place we can move the code there so it's more clear where it's utilized. it also allows us to encapsulate these functions so the exposed api is succinct.

modules/async-icq/utils/cache_ctx.go Outdated Show resolved Hide resolved
@jtieri
Copy link
Member

jtieri commented Nov 6, 2023

hey @VitalyVolozhinov wanted to ping you and see if there is an update on this? if i don't hear back from you I can address the couple nit picks that i mentioned and we can get this merged.

thanks again for your contributions!

@VitalyV1337
Copy link
Contributor Author

Thank you for reminding me about this! I've updated the code as per suggestion. Thank you!

@jtieri
Copy link
Member

jtieri commented Nov 6, 2023

Thank you for reminding me about this! I've updated the code as per suggestion. Thank you!

and thank you again for your contributions, they are greatly appreciated 🙂

@jtieri jtieri added the async-icq Label for items related to the async-icq module label Nov 6, 2023
@jtieri jtieri changed the title Queries that panic should return ack-err fix!: queries that panic should return ack-err Nov 6, 2023
@jtieri
Copy link
Member

jtieri commented Nov 6, 2023

These changes should be included in the next minor releases of async-icq as they are state breaking.

@jtieri jtieri added the BACKPORT Backport into maintained branches label Nov 6, 2023
@jtieri jtieri merged commit 83f866e into cosmos:main Nov 7, 2023
6 checks passed
mergify bot pushed a commit that referenced this pull request Nov 7, 2023
* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go
mergify bot pushed a commit that referenced this pull request Nov 7, 2023
* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go
mergify bot pushed a commit that referenced this pull request Nov 7, 2023
* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go
jtieri added a commit that referenced this pull request Nov 8, 2023
…-err` (#137)

* fix!: queries that panic should return `ack-err` (#86)

* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go

* fix: remove demo simapp code that was never backported

---------

Co-authored-by: Vitaly Volozhinov <60775046+VitalyVolozhinov@users.noreply.github.com>
Co-authored-by: jtieri <justin@thetieris.com>
jtieri added a commit that referenced this pull request Nov 8, 2023
…-err` (#138)

* fix!: queries that panic should return `ack-err` (#86)

* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go

* fix: remove demo simapp code that was never backported

---------

Co-authored-by: Vitaly Volozhinov <60775046+VitalyVolozhinov@users.noreply.github.com>
Co-authored-by: jtieri <justin@thetieris.com>
jtieri added a commit that referenced this pull request Nov 8, 2023
…-err` (#139)

* fix!: queries that panic should return `ack-err` (#86)

* adding ApplyFuncIfNoError

* Moving cache_ctx.go to the keeper

* updating comment

* chore: address linter errors

---------

Co-authored-by: jtieri <justin@thetieris.com>
(cherry picked from commit 83f866e)

# Conflicts:
#	modules/async-icq/testing/demo-simapp/app/export.go

* fix: remove demo simapp code that was never backported

---------

Co-authored-by: Vitaly Volozhinov <60775046+VitalyVolozhinov@users.noreply.github.com>
Co-authored-by: jtieri <justin@thetieris.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async-icq Label for items related to the async-icq module BACKPORT Backport into maintained branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If a query panics an error acknowledgement is not sent.
2 participants