[Transfer App] Refine return type of `process_recv_packet_execute`

[Farhad-Shabani]

Problem Statement

The current implementation of the process_recv_packet_execute in the relay module returns a Result<ModuleExtras, (ModuleExtras, TokenTransferError)> type which is not conventional and may cause confusion to future maintainers.

Proposal

It is suggested to update the implementation to return Result<ModuleExtras, TokenTransferError> directly, where TokenTransferError contains all the necessary error information. If ModuleExtras is needed to handle the error case, it can be returned as part of the error variant.

[plafer]

The problem with this suggested signature is that all error variants of TokenTransferError would need to have the logs in them, since we always want the logs, no matter the error that occurred. And at the call site, you need to match on every error one way or another to get those logs out.

[Farhad-Shabani]

all error variants of TokenTransferError would need to have the logs in them

IIUC, I don't think we need to do so. There is just needed to add one specific variant for this case (say, TokenTransferError::CannotMintCoins { extras }). Did you look into the implementation in #504?

[plafer]

But then you're implicitly saying that CannotMintCoins is the only viable variant to return, since all other variants will be throwing logs away.

[Farhad-Shabani]

Following our discussion, we have reached the conclusion that this type is the most logical output type considering the existing design and the interactions with our error, event, and logging handling mechanisms. However, with your permission @palfer, I would like to keep this issue open as we refine our designs, perhaps we find a way to address it. I noticed the way we gather events and logs differs from the IBC-go side. Perhaps exploring this disparity may assist us in resolving this issue as well.

[plafer]

yes I'm okay with keeping this issue open as the signature of process_recv_packet_execute indeed is not idiomatic.