Add log message for debug error failed acknowledgement errors

[GNaD13]

Description

closes: #2725

Log error if failed acknowledgement errors for debug

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

[codecov-commenter]

Codecov Report

Merging #3077 (bdb49f3) into main (682ba55) will decrease coverage by 0.03%.
The diff coverage is 70.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3077      +/-   ##
==========================================
- Coverage   78.60%   78.57%   -0.03%     
==========================================
  Files         177      177              
  Lines       12398    12414      +16     
==========================================
+ Hits         9745     9754       +9     
- Misses       2227     2231       +4     
- Partials      426      429       +3     

Impacted Files	Coverage Δ
modules/apps/transfer/ibc_module.go	64.81% <40.00%> (-0.16%)	⬇️
modules/apps/transfer/keeper/relay.go	90.63% <60.00%> (ø)
...les/apps/27-interchain-accounts/host/ibc_module.go	91.11% <100.00%> (+1.11%)	⬆️
...s/apps/27-interchain-accounts/host/keeper/relay.go	71.08% <100.00%> (ø)
cmd/build_test_matrix/main.go	55.00% <0.00%> (-2.45%)	⬇️

[crodriguezvega]

Thank you for working on this, @GNaD13! I left a bunch of nits and a couple of questions.

[GNaD13]

Thank you for working on this, @GNaD13! I left a bunch of nits and a couple of questions.

Hi @crodriguezvega , I have fixed it. Thanks for your review.

[chatton]

Hi @GNaD13, thanks for the work on this! I have a few suggestions about how to structure the logging.

I think it is preferable to reduce the number of locations that we are directly logging things, and prefer wrapping errors with additional context where possible.

Taking the transfer module as an example, I think the application itself should log errors from the callback just here.

This way we could update your log statements to become return sdkerrors.Wrapf(err, "additional information") and then log them in one place after im.keeper.OnRecvPacket is called.

I think the same approach can be taken with the interchain accounts host module, we can return wrapped errors and log them in the host module.

What do you think?

cc @colin-axner @damiannolan

[GNaD13]

Hi @GNaD13, thanks for the work on this! I have a few suggestions about how to structure the logging.

I think it is preferable to reduce the number of locations that we are directly logging things, and prefer wrapping errors with additional context where possible.

Taking the transfer module as an example, I think the application itself should log errors from the callback just here.

This way we could update your log statements to become return sdkerrors.Wrapf(err, "additional information") and then log them in one place after im.keeper.OnRecvPacket is called.

I think the same approach can be taken with the interchain accounts host module, we can return wrapped errors and log them in the host module.

What do you think?

cc @colin-axner @damiannolan

Hi @chatton , thanks for your reviewed. I've fixed it

[chatton]

LGTM Thanks @GNaD13

[colin-axner]

Overall LGTM! Thanks @GNaD13

I left notes on instances where I thought changes might be unnecessary, please let me know if you have any questions

[GNaD13]

Overall LGTM! Thanks @GNaD13

I left notes on instances where I thought changes might be unnecessary, please let me know if you have any questions

Hi sir @colin-axner , thanks for your reviewed. I've fixed it.

[colin-axner]

Thank you @GNaD13! I love to see our errors/debugging get easier! 🙌

I left one more suggestion, but otherwise looks perfect

[damiannolan]

🙏

[colin-axner]

The branch will need to be brought up to date with main to be automerged