[ETCM-252] faucet - handle account #783

biandratti · 2020-11-06T17:40:22Z

Description

The Faucet assumes that the ops user should configure an account on the faucet.keystore-dir before start. When starting the “faucet”, it automatically loads the account, and prepares the context to be used.

Proposed Solution

When starting the Faucet, it initializes an actor that handles the account. I added a supervisor that monitors the actor that handles the account.

We have 3 cases:

If it can't load the wallet, the app shutdown automatically.
While trying to load the wallet with the configuration. At this moment the wallet is disable. So the status of the "faucet" is “FaucetUnavailable”.
When the wallet is loaded, the status changes, now it is WalletAvailable and we can use the "faucet" with normality.

Important Changes Introduced

When the wallet is loaded successfully, the status of "faucet" changed, now it is WalletAvailable.

…-252-faucet-handle-account

mmrozek · 2020-11-12T14:05:48Z

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

+  var wallet: Wallet = _
+
+  override def preStart(): Unit = {
+    log info "||=== Faucet Handler Hook ===||"


Should we log that?

log removed, it is not necessary.

mmrozek · 2020-11-12T14:06:38Z

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

+      walletService.getWallet.runSyncUnsafe() match {
+        case Left(error) =>
+          log.error(s"Couldn't initialize wallet - error: $error")
+        //context become unavailable


I think we should kill the faucet or try to recover

This one is still not addressed. We could end up with the "running" but nonworking faucet. The better idea is to kill the whole app. It will be easier to notice (ex. in metrics)

I like this idea. So, I separated 3 cases.
1_ if it can't load the wallet, the app shutdown.
2_ While trying to load the wallet with the configuration. At this moment the wallet is disable “FaucetUnavailable”.
3_ When the wallet is loaded, the status is WalletAvailable and we can use the faucet with normality.

If this approach results ok, I will add this description in the documentation.
What do you think?

I have pending another task of improvements, unit test in the supervisor actor.

I think it is a good direction

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

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

…-252-faucet-handle-account

mmrozek

LGTM

mirkoAlic · 2020-11-18T14:12:57Z

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

+  import FaucetHandler.FaucetHandlerMsg._
+  import FaucetHandler.FaucetHandlerResponse._
+
+  var wallet: Wallet = _


why not context become available(wallet)?

mirkoAlic · 2020-11-18T15:16:10Z

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

+  implicit lazy val actorTimeout: Timeout = Timeout(config.responseTimeout)
+  implicit val ec: ExecutionContext = system.dispatcher
+
+  private def faucetHandler(): Task[ActorRef] =


maybe this could be something we receive from params of via implementing a trait. I think it will help when we added tests for the Service.

mirkoAlic · 2020-11-18T15:16:50Z

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

-    case Left(err) =>
-      throw new RuntimeException(s"Cannot unlock wallet for use in faucet (${config.walletAddress}), because of $err")
-  }
+class FaucetRpcService(config: FaucetConfig)(implicit system: ActorSystem) extends RetrySupport with Logger {


// TODO: Add unit tests for this class. (it could be done in a different task)

Add task: ETCM-395

mirkoAlic · 2020-11-18T15:17:33Z

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

-    ByteString(rlp.encode(stx.tx.toRLPEncodable))
+  def status(statusRequest: StatusRequest): ServiceResponse[StatusResponse] =
+    faucetHandler()
+      .flatMap(handler => handler.askFor[Any](FaucetHandlerMsg.Status))


what happens if we can't reach the actor? i mean in terms of response...

Pending task: ETCM-396

mirkoAlic · 2020-11-18T15:20:21Z

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

+        _ ->
+          log.info("actor system finished")
+      )(system.dispatcher),
+      1.seconds


why not use the pre-existing mantis config value?

# timeout for shutting down the ActorSystem shutdown-timeout = "15.seconds"

i mean as in src/main/scala/io/iohk/ethereum/nodebuilder/NodeBuilder.scala:

trait ShutdownHookBuilder { self: Logger => ... lazy val shutdownTimeoutDuration: Duration = Config.shutdownTimeout ... }

mirkoAlic · 2020-11-18T15:30:24Z

src/test/scala/io/iohk/ethereum/faucet/jsonrpc/WalletServiceSpec.scala

+
+    val mockRpcClient = mock[RpcClient]
+    val mockKeyStore = mock[KeyStore]
+    val config: FaucetConfig =


Suggested change

val config: FaucetConfig =

val config: FaucetConfig =

FaucetConfig(

walletAddress = wallet.address,

walletPassword = "",

txGasPrice = 10,

txGasLimit = 20,

txValue = 1,

rpcAddress = "",

keyStoreDir = "",

minRequestInterval = 10.seconds,

handlerTimeout = 10.seconds,

responseTimeout = 10.seconds,

supervisor = mock[SupervisorConfig]

)

mirkoAlic · 2020-11-18T15:31:15Z

src/test/scala/io/iohk/ethereum/faucet/jsonrpc/WalletServiceSpec.scala

+    val mockRpcClient = mock[RpcClient]
+    val mockKeyStore = mock[KeyStore]
+    val config: FaucetConfig =
+      FaucetConfig(


^ far more readable if someone is using just github to check the code, or is in a server environment.

mirkoAlic · 2020-11-18T15:39:29Z

src/test/scala/io/iohk/ethereum/faucet/FaucetHandlerSpec.scala

+    with NormalPatience {
+
+  "Faucet Handler" - {
+    "without wallet unlocked" - {


what about the scenario about Faucet initialization succeeded?

mirkoAlic · 2020-11-18T15:41:57Z

src/test/scala/io/iohk/ethereum/faucet/FaucetHandlerSpec.scala

+  "Faucet Handler" - {
+    "without wallet unlocked" - {
+
+      "should try to unlock the Wallet if it is not initialized but decryption failed" in new TestSetup {


For me this test should like:
"Faucet Handler" + "without wallet unlocked" + "should not respond in case wallet unlock fails"

mirkoAlic · 2020-11-18T15:42:36Z

src/test/scala/io/iohk/ethereum/faucet/FaucetHandlerSpec.scala

+  "Faucet Handler" - {
+    "without wallet unlocked" - {
+
+      "should try to unlock the Wallet if it is not initialized but decryption failed" in new TestSetup {


Suggested change

"should try to unlock the Wallet if it is not initialized but decryption failed" in new TestSetup {

"should not respond in case wallet unlock fails" in new TestSetup {

…-252-faucet-handle-account

mirkoAlic · 2020-11-19T11:29:28Z

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

+  val name = "FaucetSupervisor"
+}
+
+class FaucetSupervisor(walletRpcClient: WalletService, config: FaucetConfig, shutdown: () => Unit)(implicit


I think is better to directly pass the Handler, i mean, why the supervisor needs to know about the WalletService?

mirkoAlic · 2020-11-19T11:31:19Z

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

+
+  lazy val handlerTimeout: Timeout = Timeout(faucetConfig.handlerTimeout)
+
+  def faucetHandler()(implicit system: ActorSystem): Task[ActorRef] = {


(minor) selectFaucetHandler IMO in that way is more obvious that is a function, and what is doing

mirkoAlic · 2020-11-19T11:37:04Z

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

+import io.iohk.ethereum.faucet.{FaucetConfigBuilder, FaucetHandler, FaucetSupervisor}
+import monix.eval.Task
+
+trait FaucetHandlerBuilder {


FaucetHandlerSelector ?

biandratti added 10 commits November 6, 2020 11:03

add faucet actors

9ac6acc

add supervisor config

bfe1337

change test name

c2e85fc

change status from faucet

cd063c9

add wallet service spec

cf87c09

add test from actor

ba59a0e

add more test

6f149e7

add new properties

fbe9bf5

fix properties

70080c4

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

5c2f1d5

…-252-faucet-handle-account

biandratti changed the title ~~WIP [ETCM-252] faucet - handle account~~ [ETCM-252] faucet - handle account Nov 12, 2020

biandratti requested review from mirkoAlic, mmrozek and lemastero November 12, 2020 13:01

mmrozek reviewed Nov 12, 2020

View reviewed changes

biandratti added 3 commits November 12, 2020 16:55

add changes from faucet rpc service

f4cf2a1

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

1382a8a

…-252-faucet-handle-account

faucet shutdown when I can't load the wallet

598391c

biandratti requested a review from ntallar November 16, 2020 16:25

remove todo of name

44c0753

mmrozek approved these changes Nov 17, 2020

View reviewed changes

mirkoAlic reviewed Nov 18, 2020

View reviewed changes

add faucet handler builder

cfde4d0

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

biandratti added 2 commits November 18, 2020 17:08

fix config

fd985d7

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

63ce0f0

…-252-faucet-handle-account

biandratti merged commit aaf3c85 into develop Nov 18, 2020

mirkoAlic deleted the ETCM-252-faucet-handle-account branch November 19, 2020 11:25

mirkoAlic reviewed Nov 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-252] faucet - handle account #783

[ETCM-252] faucet - handle account #783

biandratti commented Nov 6, 2020 •

edited

Loading

mmrozek Nov 12, 2020

biandratti Nov 16, 2020

mmrozek Nov 12, 2020

mmrozek Nov 13, 2020

biandratti Nov 16, 2020

mmrozek Nov 17, 2020

mmrozek left a comment

mirkoAlic Nov 18, 2020 •

edited

Loading

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 18, 2020

biandratti Nov 18, 2020

mirkoAlic Nov 19, 2020 •

edited

Loading

mirkoAlic Nov 19, 2020 •

edited

Loading

mirkoAlic Nov 19, 2020 •

edited

Loading

-    val config: FaucetConfig =
+    val config: FaucetConfig =
+      FaucetConfig(
+        walletAddress = wallet.address,
+        walletPassword = "",
+        txGasPrice = 10,
+        txGasLimit = 20,
+        txValue = 1,
+        rpcAddress = "",
+        keyStoreDir = "",
+        minRequestInterval = 10.seconds,
+        handlerTimeout = 10.seconds,
+        responseTimeout = 10.seconds,
+        supervisor = mock[SupervisorConfig]
+      )

	"should try to unlock the Wallet if it is not initialized but decryption failed" in new TestSetup {
	"should not respond in case wallet unlock fails" in new TestSetup {


		lazy val handlerTimeout: Timeout = Timeout(faucetConfig.handlerTimeout)

		def faucetHandler()(implicit system: ActorSystem): Task[ActorRef] = {

[ETCM-252] faucet - handle account #783

[ETCM-252] faucet - handle account #783

Conversation

biandratti commented Nov 6, 2020 • edited Loading

Description

Proposed Solution

Important Changes Introduced

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmrozek left a comment

Choose a reason for hiding this comment

mirkoAlic Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirkoAlic Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

mirkoAlic Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

mirkoAlic Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

biandratti commented Nov 6, 2020 •

edited

Loading

mirkoAlic Nov 18, 2020 •

edited

Loading

mirkoAlic Nov 19, 2020 •

edited

Loading

mirkoAlic Nov 19, 2020 •

edited

Loading

mirkoAlic Nov 19, 2020 •

edited

Loading