[r2r] tendermint missing implementations

[onur-ozkan]

• Finish TODO notes (except ones came from dev branch later, and https://github.com/KomodoPlatform/atomicDEX-API/blob/f3b6ca9e1a1a8e0fabb1a23d9ed6e542474c5189/mm2src/coins/tendermint/tendermint_coin.rs#L407-L410 which Alina working on)
• tx_history_v2 implementation for tendermint
• fix avg_blocktime / avg_block_time confusion

breaking change
for tendermint configuration in coins file, avg_block_time is now called avg_blocktime.

[shamardy]

Great work as always, First review iteration.

[shamardy]

get_rpc_client only wraps rpc_client function. My personal preference in such cases is to create a seperate trait for the rpc_client fn and implement it for TendermintCoin then add this trait as a requirement for implementing TendermintTxHistoryOps. This way we have only one function in the code that does the required functionality.

[laruh]

@shamardy I feel its kinda related with my refactoring rpc_clients in tendermint task.
So we usually keep vector of clients and we have to iterate it every time to send request (this issue).
We have such problems like here in web3, in z_rpc, in tendermint_coin.
So what I want to say, may be we should have a separate module for trait with Ops to get client or iterate over urls, if client doesn't work.
Every time, when we need to impl this trait, we can create a special structure that contains two fields: one active client and vec of urls. So we can use this structure as field for TendermintCoinImpl (for example) or use it instead of Vec<CompactTxStreamerClient<Channel>> from z_rpc.
There is a bit another way. Every time we can create a special trait for every coin with structure that implements it.
@ozkanonur @sergeyboyko0791 what do you think?

[sergeyboyko0791]

To be honest, I didn't quite understand what you mean, so will try to explain my opinion:
Regarding #1480, I think it could be solved within a Rpc client structure, not outside of it:

struct Web3Transport {
  // An identifier of the URI
  active_rpc_id: usize,
  id: Arc<AtomicUsize>,
  uris: Vec<http::Uri>,
  event_handlers: Vec<RpcTransportEventHandlerShared>,
}

Same approach could be done for z_rpc:

struct ZRpcClient {
  active_channel_id: usize,
  channels: Vec<CompactTxStreamerClient<Channel>>,
}

@laruh does this make sense? Or am I being too naive?

[shamardy]

So what I want to say, maybe we should have a separate module for trait with Ops to get client or iterate over urls, if client doesn't work.

As I understand, you want a trait separate from tendermint for rpc clients that have 2 functions get_rpc_client, iterate_over_urls then implement this trait for both zcoin and tendermint. This actually a good idea :)

@ozkanonur do you think this is related to this issue #1480? If so we need to mention this required refactor in the issue and it shouldn't be done here :)

[laruh]

To be honest, I didn't quite understand what you mean, so will try to explain my opinion: Regarding #1480, I think it could be solved within a Rpc client structure, not outside of it:

struct Web3Transport {
  // An identifier of the URI
  active_rpc_id: usize,
  id: Arc<AtomicUsize>,
  uris: Vec<http::Uri>,
  event_handlers: Vec<RpcTransportEventHandlerShared>,
}

Same approach could be done for z_rpc:

struct ZRpcClient {
  active_channel_id: usize,
  channels: Vec<CompactTxStreamerClient<Channel>>,
}

@laruh does this make sense? Or am I being too naive?

Sorry, now I see, my explanation was a mess. will provide my code examples.
So now we usually have smth like that: Vec<CompactTxStreamerClient<Channel>> (not a part of any structure), Vec<Web3TransportNode> (one of the fields from struct Web3Transport), Vec<HttpClient> (filed from struct TendermintCoinImpl).
Onur suggested using only one alive client for requests instead of iteration over clients every time.
So, I suggest looking at the current implementation of the clients vector as

struct TendermintRpcClient {
    rpc_urls: Vec<String>,
    rpc_client: AsyncMutex<HttpClient>,
}

struct ZRpcClient {
    lightwalletd_urls: Vec<String>,
    rpc_client: AsyncMutex<CompactTxStreamerClient<Channel>>,
}

struct Web3Node {
    urls: Vec<String>,
    node: AsyncMutex<Web3TransportNode>,
}

We can replace vectors of clients with rpc structures that contain necessary fields.
And then we could have a common trait named like RpcCommonOps with methods get_rpc_client (to get alive client) and iterate_over_urls (to iterate over urls if request for alive client isnt successful).
As a result, we can have a new rule in our project, when we need rpc client to send requests in different coin implementations: Create structure that contains one client and vector of urls. Implement trait RpcCommonOps for this structure.
In addition, we have to wrap rpc_client field into Mutex to be able to change it, if its dead.
In my example I use AsyncMutex, bcz I need the MutexGuard to be Send in tendermint implementation.

[laruh]

So what I want to say, maybe we should have a separate module for trait with Ops to get client or iterate over urls, if client doesn't work.

As I understand, you want a trait separate from tendermint for rpc clients that have 2 functions get_rpc_client, iterate_over_urls then implement this trait for both zcoin and tendermint. This actually a good idea :)

@ozkanonur do you think this is related to this issue #1480? If so we need to mention this required refactor in the issue and it shouldn't be done here :)

Yeah, we should have a main rule in the project, when we have a deal with rpc clients. Just during optimization in Tendermint Coin, I had a look at another vectors with rpc clients and decided to bring up this topic here.

[onur-ozkan]

Trait implementation for all rpc client handlings is a good idea. So we will be enforced by compiler to keep rpc client logics (at least args, function names and result types) as same as others.

[shamardy]

Instead of getting the protocol type from config, why not add fn requires_remapping(&self) -> bool to CoinWithTxHistoryV2 and return true for Tendermint coin/asset.

[sergeyboyko0791]

There also could be CoinWithTxHistoryV2::remap_tx_if_required(&self, details: &mut TransactionDetails). So this remapping logic is performed on the coin's level.
What do you think?

[shamardy]

Agree, this is better :)

[onur-ozkan]

There also could be CoinWithTxHistoryV2::remap_tx_if_required(&self, details: &mut TransactionDetails). So this remapping logic is performed on the coin's level. What do you think?

This is actually pretty good idea. However, we already have mapping for all v2 history implementations. So I will need to do this change for all. Should I cover it in this PR? I left a TODO note here
https://github.com/KomodoPlatform/atomicDEX-API/blob/452fc50b38ef4b73f6d20c058fb1b637a7885d0d/mm2src/coins/my_tx_history_v2.rs#L439-L443

Please let me know if this change is blocker and if I should cover it.

[shamardy]

This is a non-blocker, you can do it in the next PR for sure.

[artemii235]

One more review iteration

[sergeyboyko0791]

All the previous comments were solved!
A few more questions and suggestions

[sergeyboyko0791]

Next review iteration

[shamardy]

Great Work 🔥, Only one non-blocker

[sergeyboyko0791]

Found two more notes

[artemii235]

🔥

[sergeyboyko0791]

Looks good to me! All notes have been solved, thank you!