event attribute value for sdk.AttributeKeyModule is incorrectly determined with ibc-go Msg types

[colin-axner]

Summary

The new baseapp.createEvents() function makes an assumption that does not hold for ibc-go. It assumes the module name is in the second element of a Msg URL. For ibc-transfer this would return "applications" which will be the same value returned for the interchain accounts module.

Proposal

I don't have a proposal at the moment. Parsing the module name correctly from the Msg URL seems likely impossible. I'd suggest either investigating whether the emission of this value is useful or investigating whether the module name can be obtained in another fashion not via the msg URL

I don't believe this will cause any immediate issues, but given that events are crucial to IBC, it would seem odd to leave this behaviour unchanged in the long term and could potentially lead to hard to debug issues

[julienrbrt]

Yes, this was a concern I had when implementing this feature. Unfortunately, I could not figure out how to implement that nicely. We could have a rule for ibc, but that is an ugly fix.

[colin-axner]

Yea, I'd be opposed to a special rule for ibc. Doesn't seem practical. Do we know what users use this event attribute for? I'm wondering if we can deprecate it and ask them to use the full msg url

[tac0turtle]

how come ibc uses applications here? seems like it should be ibc.transfer?

[colin-axner]

It's just how the proto files have been laid out, changing the existing URL would be too difficult to do at this point since it would break Anys

[tac0turtle]

I think in this case emitting applications automatically and ibc module name manually is the solution here. This cleans up a lot of boiler plate for users.

[tac0turtle]

@colin-axner want to look at #14352. Its a bit hacky but I think its a safer assumption than position 1 of message url

[crodriguezvega]

@colin-axner want to look at #14352. Its a bit hacky but I think its a safer assumption than position 1 of message url

@colin-axner is on holidays now. @damiannolan would you please be able to take a look?

[damiannolan]

I think this might be problematic to rely on the store keys for module names. The ibc core stack of submodules(02-client, 03-connection, 04-channel) use the store key defined by 24-host here which is "ibc" (transfer uses its own dedicated store).

However, the events emitted from each of these submodules seem to be a concatenation of host.ModuleName + _ + SubModuleName. See here. This would break relayers afaik, and I am thinking that the current moduleName changes in the 0.47 alpha tags could be the cause for the e2e's failing on ibc-go.

Hermes seems to rely on the module name -> https://github.com/informalsystems/hermes/blob/5cdae2b1ae9e74c39627a645ab1c7523897cb3d6/crates/relayer/src/event/monitor.rs#L148-L158... but I can't seem to find it atm in the go relayer code.

[damiannolan]

Given that the module name is event value is constructed for:

• 02-client
• 03-connection
• 04-channel

as host.ModuleName + _ + SubModuleName, I don't see how there is a solution (even a hacky solution) for baseapp to be able to get at this information. And I think changing this would add a massive amount of complexity and overhead for relayers to be compatible between versions.

I understand cleaning this up and reducing the amount of boilerplate for module devs would be great, but it seems like it could be more hassle than its worth.

cc. @jackzampolin @adizere

[jackzampolin]

Just to chime in here, we would need to start adding switches in all our event parsing code for this and it would be pretty ugly. If there is a strong need to do this then I understand however this is a rough breaking change for relayers.

[tac0turtle]

the original change was part of a movement to clean up boilerplate. Modules can still create the same events as before, they may be larger. I don't think there is a fix for ibc-go other than they have to emit their own events. We should add a check to see if a module has emitted events or we have to. cc @julienrbrt .

[damiannolan]

Thanks @tac0turtle, I think that's probably the best solution for now.