feat: improve sending error handling

[genaris]

Currently, when an outbound message is sent as a result of calling a module/API method and a transport error happens, an exception is thrown without any retry mechanism (unless handled externally by the outbound transporter object). In the particular case of Basic Messages, when such situation happens, a record is created but there are no means from the calling application to know its id and delete it or mark it as failed in the UI.

This PR addresses this issue by throwing in BasicMessageModule.sendMessage a new kind of error: MessageSendingError, which includes the message itself and the associated record. The reason of adding the message is to open the possibility for the calling application to retry sending if it wants (not something so straightforward though, but feasible).

The same strategy could potentially be extended to methods from other modules that also create records and send messages in a single step (e.g. sendQuestion, proposeCredential, offerCredential).

Signed-off-by: Ariel Gentile [email protected]

[codecov-commenter]

Codecov Report

Merging #1045 (ca74456) into main (0d14a71) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
- Coverage   88.74%   88.73%   -0.02%     
==========================================
  Files         522      523       +1     
  Lines       12156    12176      +20     
  Branches     1913     1913              
==========================================
+ Hits        10788    10804      +16     
- Misses       1364     1368       +4     
  Partials        4        4              

Impacted Files	Coverage Δ
packages/core/src/error/MessageSendingError.ts	100.00% <100.00%> (ø)
packages/core/src/error/index.ts	100.00% <100.00%> (ø)
.../src/modules/basic-messages/BasicMessagesModule.ts	94.73% <100.00%> (+1.40%)	⬆️
...les/basic-messages/services/BasicMessageService.ts	93.75% <100.00%> (+1.15%)	⬆️
packages/core/src/error/IndySdkError.ts	40.00% <0.00%> (-60.00%)	⬇️
packages/core/src/storage/IndyStorageService.ts	94.26% <0.00%> (-0.82%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[berendsliedrecht]

Nice change! Seems rather useful for any non-happy flow.

-- I also like the TSDocs you added :D

[TimoGlastra]

Just thinking here, but what if instead of handing this in the module layer, we hanle this in the message layer? That way we don't have to support this in every module, and new modules get this functionality out of the box.

One issue is that the module doesn't have access to the associated record, but maybe we can add some optional metadata to the outbound message (such as associatedRecord).

[genaris]

Just thinking here, but what if instead of handing this in the module layer, we hanle this in the message layer? That way we don't have to support this in every module, and new modules get this functionality out of the box.

One issue is that the module doesn't have access to the associated record, but maybe we can add some optional metadata to the outbound message (such as associatedRecord).

Good idea! Other modules will still need to add associatedRecord when calling to MessageSender, but their logic will remain the same. And fewer lines modified means less potential bugs being introduced :-)

I think the simplest approach for this would be adding associatedRecord as optional field in options for MessageSender.sendMessage().

[genaris]

Just thinking here, but what if instead of handing this in the module layer, we hanle this in the message layer? That way we don't have to support this in every module, and new modules get this functionality out of the box.
One issue is that the module doesn't have access to the associated record, but maybe we can add some optional metadata to the outbound message (such as associatedRecord).

Good idea! Other modules will still need to add associatedRecord when calling to MessageSender, but their logic will remain the same. And fewer lines modified means less potential bugs being introduced :-)

I think the simplest approach for this would be adding associatedRecord as optional field in options for MessageSender.sendMessage().

So I finally added it to OutboundMessage because I think it makes more sense there (and also it could be useful to use in other flows like message reception and handlers). In BasicMessageModule I've manually set outboundMessage.associatedRecordId after calling createOutboundMessage because I would need to refactor that helper method that is currently used everywhere in the framework. I think it's worth to refactor it after 0.3.0 release.

[TimoGlastra]

Adding the id but not the type can make it hard to get the record type in a generic context. A possibility is attachign the whole record, but I like the id appraoch better.. What do you think?

[genaris]

I was also hesitating on using the whole record or just the id. Opted to go for the id in order to make this whole outbound message more 'lightweight' and considering that the context would know the type of record to search for. But I don't have a very strong opinion on this. Can change to the whole record if we foresee that it's more convenient.

[TimoGlastra]

Yeah I think my preference would go to attaching the record