return an error if transaction history futures initialization fails

[shamardy]

As explained by @onur-ozkan

Just checked the source code, activation and deactivation operations are relying on the CoinsContext Mutex.

e.g.,:

activation requested -> TendermintCoin initialized -> spawning tx/history / balance streaming futures -> locked(no parallel execution allowed here) coins context and added TendermintCoin in there -> success result

However, spawned futures can end up unexpectedly because there is no guarantee that they will be successfully executed/initialized before we add their coin into CoinsContext. This means that a spawned future could encounter errors or panics after we add the coin to the context and return a success result.

ref. #1978 (comment), #1978 (comment), #1978 (comment)

[mariocynicys]

This isn't a problem though since tx history is an auxiliary feature, we don't wanna disable the coin just because one of its streamers died? no?

[onur-ozkan]

This isn't a problem though since tx history is an auxiliary feature, we don't wanna disable the coin just because one of its streamers died? no?

We should return an error when the core/main logic fails in the background thread (e.g., see how it's done in the event streaming implementations) instead of saying "coin is activated successfully".

[mariocynicys]

We should return an error when the core/main logic fails in the background thread (e.g., see how it's done in the event streaming implementations) instead of saying "coin is activated successfully".

Yeah we might return an error via the activation request as there is no channel to display this error to the user from. But i didn't get whether we are for deactivating the coin or not (which i think we shouldn't). But also returning a plain error while keeping the coin active won't make any sense for whose requesting.

In the light of #2172, coin activations has nothing to do with initializing event streamer. One would need to send a new request for initializing the streamer after having activated the coin.

[onur-ozkan]

Yeah we might return an error via the activation request as there is no channel to display this error to the user from. But i didn't get whether we are for deactivating the coin or not (which i think we shouldn't). But also returning a plain error while keeping the coin active won't make any sense for whose requesting.

If we are spawning a task along with some request, we should make sure the spawned task will get into the right context/state before saying "all is good" instead of ignoring the spawned task status and returning success result immediately. This is the whole idea.

In the light of #2172, coin activations has nothing to do with initializing event streamer. One would need to send a new request for initializing the streamer after having activated the coin.

I think this is unrelated with tx history implementations?

[mariocynicys]

If we are spawning a task along with some request, we should make sure the spawned task will get into the right context/state before saying "all is good" instead of ignoring the spawned task status and returning success result immediately. This is the whole idea.

we agree on that. i am talking about solutions thought. the plausible one on my mind right now is to activate the coin normally but also report on errors that occurred by trailing an errors field in the response.

I think this is unrelated with tx history implementations?

tx history is an event streamer.

[onur-ozkan]

If we are spawning a task along with some request, we should make sure the spawned task will get into the right context/state before saying "all is good" instead of ignoring the spawned task status and returning success result immediately. This is the whole idea.

we agree on that. i am talking about solutions thought. the plausible one on my mind right now is to activate the coin normally but also report on errors that occurred by trailing an errors field in the response.

Returning errors in successful requests makes things more complicated than it should. We shouldn't be overengineering this to be honest. Just return the error when there is one and that's it. What are the concerns you have on this?

I think this is unrelated with tx history implementations?

tx history is an event streamer.

With that PR I guess? Yeah, I am not really sure what is what after the changes that PR brings..