[ETCM-251] faucet healthcheck #768

biandratti · 2020-10-29T17:31:37Z

Description

Add json rpc behavior in “faucet”. So we change httpServer to jsonRpcServer.

We change the resource from sendFunds.
And add a new resource from the status of “faucet”, that today it’s return value “FaucetUnavailable”. In other tasks we complete the status from faucet.

Proposed Solution

I added a new JsonRpc controller called "JsonRpcBaseController'', it's extended for the clases JsonRpcController and FaucetJsonRpcController. It is the way to decouple both controllers.

Important Changes Introduced

Change the endpoint from the request send funds of "faucet". Before it worked with "rest", now it will work with "json rpc".
Before => [POST] {host}/faucet?address={account_id}
Today => We use json rpc with "method": "faucet_sendFunds" and account_id as parameters.
New endpoint was documented in the file "insomnia_workspace.json".
Before for the resource "send Funds'' we had a validation for the next request from the same ip address. We validated the limit time between requests form the same ip address. This validation was removed because it is not necessary, this sholud be part of DNS protection.

Testing

json Rpc regression

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetBuilder.scala

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetDomain.scala

src/main/scala/io/iohk/ethereum/jsonrpc/server/controllers/JsonRpcControllerCommon.scala

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetRpcService.scala

mmrozek

Looks nice. Minor comments only

mmrozek · 2020-11-04T17:14:18Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetJsonRpcController.scala

-      Future.successful(errorResponse(req, JsonRpcError.MethodNotFound))
+  def handleRequest: PartialFunction[JsonRpcRequest, Task[JsonRpcResponse]] = { case req =>
+    val notFoundFn: PartialFunction[JsonRpcRequest, Task[JsonRpcResponse]] = { case _ =>
+      Task(errorResponse(req, JsonRpcError.MethodNotFound))


Task.now(errorResponse(req, JsonRpcError.MethodNotFound))

Done, I applied it in other cases too.

mmrozek · 2020-11-04T17:15:20Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetRpcService.scala

@@ -32,11 +33,11 @@ class FaucetRpcService(rpcClient: RpcClient, keyStore: KeyStore, config: FaucetC
      case Right(txId) =>
        val txIdHex = s"0x${ByteStringUtils.hash2string(txId)}"
        log.info(s"Sending ${config.txValue} ETH to ${sendFundsRequest.address} in tx: $txIdHex.")
-        Future.successful(Right(SendFundsResponse(txId)))
+        Task(Right(SendFundsResponse(txId)))


mmrozek · 2020-11-04T17:15:27Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetRpcService.scala


      case Left(err) =>
        log.error(s"An error occurred while using faucet: $err")
-        Future.successful(Left(JsonRpcError.InternalError))
+        Task(Left(JsonRpcError.InternalError))


mmrozek · 2020-11-04T17:17:30Z

src/main/scala/io/iohk/ethereum/jsonrpc/server/controllers/JsonRpcBaseController.scala

+    handleFn(request)
+      .flatTap {
+        case JsonRpcResponse(_, _, Some(JsonRpcError(_, _, _)), _) =>
+          Task(JsonRpcControllerMetrics.MethodsErrorCounter.increment())


mmrozek

LGTM!

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetRpcService.scala

mirkoAlic · 2020-11-05T18:01:03Z

src/main/scala/io/iohk/ethereum/nodebuilder/NodeBuilder.scala

+    val Web3 = "web3"
+    val Net = "net"
+    val Personal = "personal"
+    val Daedalus = "daedalus"


I think we could already remove this option

but is ok to leave as it is in this PR.

I will present this change in the next task.

src/main/scala/io/iohk/ethereum/faucet/FaucetConfig.scala

…-hk/mantis into ETCM-251-faucet-healthcheck

lemastero · 2020-11-06T11:23:56Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetMethodsImplicits.scala

+import io.iohk.ethereum.jsonrpc.serialization.{JsonEncoder, JsonMethodDecoder}
+import org.json4s.JsonAST.{JArray, JObject, JString}
+
+object FaucetMethodsImplicits extends JsonMethodsImplicits {


How about a name that would tell us more what exactly those implicits do e.g. FaucetJsonCodecs ?

Right, is true, I will change the name in the next task 252.

…-251-faucet-healthcheck

lemastero · 2020-11-06T12:28:36Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetRpcService.scala

+      case Right(txId) =>
+        val txIdHex = s"0x${ByteStringUtils.hash2string(txId)}"
+        log.info(s"Sending ${config.txValue} ETC to ${sendFundsRequest.address} in tx: $txIdHex.")
+        Task.now(Right(SendFundsResponse(txId)))


I wonder can we wrap the whole sendFunds into Task?

lemastero · 2020-11-06T12:32:32Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetMethodsImplicits.scala

+
+  implicit val sendFundsRequestDecoder: JsonMethodDecoder[SendFundsRequest] = {
+    case Some(JArray((input: JString) :: Nil)) => extractAddress(input).map(SendFundsRequest)
+    case _ => Left(InvalidParams())


WDYT about adding more details. Sth like:

implicit val sendFundsRequestDecoder: JsonMethodDecoder[SendFundsRequest] = { case Some(JArray((input: JString) :: Nil)) => extractAddress(input).map(SendFundsRequest) case other => Left(InvalidParams(s"Wrong parameters $other"))

or maybe add some details like "Expected JSON array with single String".

lemastero · 2020-11-06T12:42:53Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetBuilder.scala

+
+trait ActorSystemBuilder {
+  def systemName: String
+  implicit lazy val system: ActorSystem = ActorSystem(systemName)


WDYT about passing dependencies as parameters to class rather than mixing traits?

I find mixin traits to be hard to test/mock/keep track to release properly/reason about.
Maybe it would be possible to pass ActorSystem as implicit param whenever it is needed?

lemastero · 2020-11-06T12:44:32Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetBuilder.scala

+trait FaucetJsonRpcHealthCheckBuilder {
+  self: FaucetRpcServiceBuilder =>
+
+  val faucetJsonRpcHealthCheck = new FaucetJsonRpcHealthCheck(faucetRpcService)


IMHO it would be easier to test/reason about if faucetJsonRpcHealthCheck would be passed as an argument to class rather than summon somewhere deep in the inheritance hierarchy (traits mixin).

lemastero · 2020-11-06T12:46:28Z

src/main/scala/io/iohk/ethereum/faucet/jsonrpc/FaucetBuilder.scala

+trait ApisBuilder extends ApisBase {
+  object Apis {
+    val Faucet = "faucet"
+  }


It seems that whole purpose of this trait and object inside is to have some place for constant Faucet. How about creating:

object FaucetApi { val Faucet = "faucet" }

and just use it whenever it is needed. Or pass as parameter if this can have different values.

* init refactor * add refactor from JsonRpcController * add refactor from JsonRpcController * change rpc fuacet * remove faucet api * change faucet config * fix test * change properties * add new endpoints from faucet * remove config from limit request * change name from JsonRpcBaseController * fix test * change faucet implicits * change monix task -- add now * remove configs from http * fix test * change test Co-authored-by: Piotr Paradziński <[email protected]>

biandratti added 10 commits October 27, 2020 08:43

init refactor

9ca859e

add refactor from JsonRpcController

95ac2d4

add refactor from JsonRpcController

235fb7b

change rpc fuacet

3e6390e

merge with develop

3da6e41

remove faucet api

d828788

change faucet config

2a25139

fix test

4d47de6

change properties

3c41d96

add new endpoints from faucet

8a8c524

biandratti added the BREAKS API Affects services that interacts with APIs label Nov 3, 2020

remove config from limit request

bf14b14

biandratti changed the title ~~WIP [ETCM-251] faucet healthcheck~~ [ETCM-251] faucet healthcheck Nov 3, 2020

biandratti requested review from mmrozek, lemastero and mirkoAlic November 3, 2020 15:02

mmrozek reviewed Nov 3, 2020

View reviewed changes

biandratti added 4 commits November 4, 2020 11:13

change name from JsonRpcBaseController

26886b8

merge with develop

d23da17

fix test

d6844bf

change faucet implicits

1b5c3ec

mmrozek reviewed Nov 4, 2020

View reviewed changes

change monix task -- add now

3f3f384

mmrozek approved these changes Nov 5, 2020

View reviewed changes

Merge branch 'develop' into ETCM-251-faucet-healthcheck

8f6522e

mirkoAlic approved these changes Nov 5, 2020

View reviewed changes

biandratti added 4 commits November 5, 2020 16:11

remove configs from http

3b537fd

Merge branch 'ETCM-251-faucet-healthcheck' of github.com:input-output…

31f3311

…-hk/mantis into ETCM-251-faucet-healthcheck

fix test

297dc9e

change test

1fe63fc

lemastero reviewed Nov 6, 2020

View reviewed changes

Merge branch 'develop' of github.com:input-output-hk/mantis into ETCM…

dfa90bb

…-251-faucet-healthcheck

biandratti merged commit 272e9ad into develop Nov 6, 2020

lemastero reviewed Nov 6, 2020

View reviewed changes

SoseMagarian mentioned this pull request Nov 20, 2020

[ETCM-331] Add Rate Limit for rpc requests #806

Merged

dzajkowski deleted the ETCM-251-faucet-healthcheck branch April 9, 2021 12:04

[ETCM-251] faucet healthcheck #768

[ETCM-251] faucet healthcheck #768

Uh oh!

Conversation

biandratti commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed Solution

Important Changes Introduced

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

biandratti commented Oct 29, 2020 •

edited

Loading