ics20 could emit more information on MsgTransfer

[colin-axner]

Summary

We only emit sender/reciever in ics20 send transfer. We could emit amount/denom/metadata as well

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[Anmol1696]

Can take this up

[crodriguezvega]

Can take this up

Yes! Thank you, @Anmol1696!

[Anmol1696]

@crodriguezvega in the PR i realized there are not much tests for checking the events itself for any of msg emiters. Do you think it would make sense to add unit tests for testing events as well for each of the msg types. I am think of it more from catching regression perspecitve more than anything else.

I tried to add a way for testing the event in this PR, maybe we can make it better and add it everywhere.

[crodriguezvega]

@crodriguezvega in the PR i realized there are not much tests for checking the events itself for any of msg emiters. Do you think it would make sense to add unit tests for testing events as well for each of the msg types. I am think of it more from catching regression perspecitve more than anything else.

I tried to add a way for testing the event in this PR, maybe we can make it better and add it everywhere.

Yes, I think it makes sense to test the events emitted (at least the ones that external parties, like relayers depend upon). Thanks for adding that to the PR. However, as you can see it requires quite some boilerplate code to be able to test the events, since there's no first-class support for it in the testing package. Before testing other events I think it would be good to hear @colin-axner's opinion as well, in case there's a way to improve the testing package to make this easier and reduce the boilerplate code needed in the tests. Alternatively we can also rely on the e2e tests (which use a relayer) to test that the events get emitted. Since modifying events is a non state-machine breaking change, we can also easily improve upon them and make a patch release.

[Anmol1696]

Hmm that does make sense, for this PR i did not try to optimize the boilerplate for testing events, ideally if this is something of interest, we can optimize it and create helper functions as well.
I am a bigger proponent of e2e tests, but as events are becoming more and more important, maybe having them checked could be a nice to have, if we figure out a good approach to it.

[Anmol1696]

FYI @crodriguezvega I tried to make the boilerplate for checking the events againts an expected map of map in the same PR, https://github.com/cosmos/ibc-go/pull/2643/files#diff-ae66bd29b0c68b16e3fc64fb2d8c88dcc90ef4ed6c1f67be36baf8d693473f8eR9, maybe something like this we could use in a more generic way?

[Anmol1696]

Should i open up a separate issue with more details for this event testing issue with a broader scope?

[colin-axner]

Yes, event testing can go in a separate issue and ideally smaller issues are opened for each body of work that should have an associated pr. I find it easier to review/merge pr's that only affect a handful of files as opposed to refactoring all our modules event testing in one pr. For example, if you add a test for testing transfer events, that'd be one pr, same goes for core ibc send packet, recv packet etc