Skip to content

Conversation

@biandratti
Copy link
Contributor

@biandratti biandratti commented Nov 16, 2020

Description

The faucet uses RpcClient to connect with the node. We had a RpcClient in the module “mallet”. This client is not async, it makes a “await” (blockin the threads) for all requests. Also it has not implemented the ssl behavior when connected with node secure (https).

Proposed Solution

  1. I add a generic RpcClient, decouple of module “mallet” (This module is pending to be removed).
  2. Add ssl behavior in the rpc client.
  3. Add a folder for testing in development. This folder is called “tls”, and too it has a script for tls generation.
  4. In faucet configuration, I separate the ssl configuration. We can have for on the one hand the context tls for the “faucet” and for another hand the tls context used for the rpc client (used to connect with the node).
  5. Coverage in class SSLContextFactory.
  6. Pretty log from rpc client.

Important Changes Introduced

Change the name of ssl properties. And add new ssl context used for rpc client from “faucet”.

Testing

ssl regression in faucet and node

@biandratti biandratti changed the title WIP [ETCM-215] faucet rpc client [ETCM-215] faucet rpc client Nov 25, 2020
@biandratti biandratti added the BREAKS CONFIG Affects the default configuration label Nov 25, 2020
@biandratti biandratti requested a review from lemastero November 25, 2020 13:00
maybeSslContext.fold(Http().defaultClientHttpsContext)(ConnectionContext.httpsClient)

def shutdown(): Unit = {
Await.ready(system.terminate(), 5.seconds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be better if e use timeout from config (ex. mantis.shutdown-timeout)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I can't find where it is used and I am not sure that this code should be able to terminate system passed to the class from outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we need this function called “shutdown()”. What do you think? Do we need it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need it. It should be done at some higher level class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function called “shutdown” was used only in the “mallet” module. But mallet now uses the class “RpcClientMallet”. This client needs the “shutdown” function.

But we have pending removal of this module in task “ETCM-423”. So I removed this function that is not used.

Copy link
Contributor

@mirkoAlic mirkoAlic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only minors, excelent job! 🚀

@biandratti biandratti merged commit 8476c04 into develop Nov 30, 2020
@dzajkowski dzajkowski deleted the ETCM-215-faucet-rpc-client branch April 9, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKS CONFIG Affects the default configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants