[CGKIELE-107] mallet #436

rtkaczyk · 2018-04-09T14:34:50Z

The minimalist CLI wallet for KEVM/IELE testnet.

Additional improvements:

a fix for sbt-verify: to regenerate checksums simply type verifyGenerate. This will create a file verify.sbt in the root dir - no need for manual copying of the output

…tps support

loverdos · 2018-04-11T07:36:17Z

build.sbt

+  "org.jline" % "jline" sha1 "dfb4e9e15e981634155ce063fa697b2b8964d507",
+  "org.scala-lang.modules" % "scala-parser-combinators" sha1 "3c1c5475ece77c41e18dd971f8f818c091e4961c",
+  // FIXME: WTF?
+  "com.google.j2objc" % "j2objc-annotations" sha1 "976d8d30bebc251db406f2bdb3eb01962b5685b3"


I did a bit of searching. This is a dependency pulled in via guava v22 (check image below) and is discussed here.

What uses guava?

Using sbt with addSbtPlugin("net.virtual-void" % "sbt-dependency-graph" % "0.9.0"):

$ sbt whatDependsOn com.google.guava guava 22.0 [info] com.google.guava:guava:22.0 [info] +-io.atomix:atomix-utils:2.1.0-beta1 [info] | +-io.atomix:atomix-cluster:2.1.0-beta1 [info] | [...] [info] +-org.consensusresearch:scrypto_2.12:1.2.0-RC3 [S] [info] | [...] [info] +-org.scorexfoundation:iodb_2.12:0.3.0 [S] [info]

@loverdos I removed the j2objc-annotations artifact from my local cache. After rebuilding, the checksum changed and it passed the verification. See also the updated PR description

loverdos · 2018-04-11T08:28:12Z

src/main/scala/io/iohk/ethereum/consensus/ethash/EthashMiner.scala

@@ -93,7 +93,7 @@ class EthashMiner(
        val startTime = System.currentTimeMillis()
        val mineResult = mine(headerHash, block.header.difficulty.toLong, dagSize, dag, miningConfig.mineRounds)
        val time = System.currentTimeMillis() - startTime
-        val hashRate = (mineResult.triedHashes * 1000) / time
+        val hashRate = if (time > 0) (mineResult.triedHashes * 1000) / time else Long.MaxValue


I was wondering if we could use a "better" (=monotonic) clock, e.g. System.nanoTime() whose purpose is to measure elapsed time. There is no strong guarantee that this is indeed going to be monotonic because it ultimately depends on the OS facilities but this is generally not a problem in Linux nowadays.

Another possible route is to not update the hash rate if we cannot reliably compute it.

Note: guava has a com.google.common.base.StopWatch class for this purpose.

I'd rather not spend too much time on this, this is not really a big problem anyway. I can add a FIXME though

loverdos · 2018-04-11T09:32:37Z

src/main/scala/io/iohk/ethereum/mallet/common/Util.scala

+  }
+
+  implicit class OptionOps[T](opt: Option[T]) {
+    def toEither[U](left: U): Either[U, T] =


There is scala.Option.toRight, so we can avoid introducing a new construct.

lol, how did I miss that? :)

LukasGasior1 · 2018-04-17T11:23:13Z

src/universal/bin/mantis

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+./bin/mantis-app -Dconfig.file=./conf/mantis.conf -Dlogback.configurationFile=./conf/logback.xml -J-Xss10M


we should also fix mantis-vm script to use mantis-app (currently it won't run vm but start mantis node)

Good point, I was actually going to explore another way with those scripts, thanks for reminding me.

LukasGasior1 · 2018-04-17T11:25:21Z

build.sbt

-        warnOnUnverifiedFiles = true,
-        warnOnUnusedVerifications = true
+        warnOnUnverifiedFiles = false,
+        warnOnUnusedVerifications = false


why this change? it seems keeping it true is more secure

false means error instead of warning. I actually didn't plan on changing this, but it happened during some fixes I applied wrt sbt-verify.

I'm not having any problems with sbt-verify now, so I wouldn't mind staying with false. WDYT?

Makes sense, I didn't know it means an error :)

LukasGasior1 · 2018-04-17T11:29:07Z

src/main/scala/io/iohk/ethereum/mallet/interpreter/AST.scala

+  }
+
+  case class Quoted(input: String) extends Literal {
+    def unqoute: String = unquote(input)


typo: unqoute -> unquote

LukasGasior1 · 2018-04-17T11:54:13Z

src/main/scala/io/iohk/ethereum/mallet/interpreter/Parameter.scala

+
+    protected val convert = {
+      case h: Hex if h.bytes.nonEmpty && h.bytes.length <= 20 =>
+        Address(h.bytes)


can we also support quoted addresses?

My first intuition was to try with quotes. Also geth console uses quoted addresses (it doesn't even accept unquoted ones)

I guess it would be easy to do, but for now I wouldn't go further than adding a TODO comment to consider this.
Firstly, I'd rather have the syntax be strict, and secondly there's supporting account aliases (which would be quoted) on the list missing features.

LukasGasior1 · 2018-04-17T12:24:23Z

src/main/scala/io/iohk/ethereum/mallet/service/RpcClient.scala

+      "jsonrpc" -> "2.0".asJson,
+      "method" -> method.asJson,
+      "params" -> args.asJson,
+      "id" -> "mallet".asJson


It doesn't really matter for Mantis, but the semantics of this field is that it's a unique request id. Maybe it would be better to just use a random value here?

I guess the idea was for it to be easily recognisable that the reqest came from mallet. How about "id" -> ("mallet_" + randomUuid()).asJson?

LukasGasior1 · 2018-04-17T12:25:43Z

verify.sbt

+  "com.github.scopt" % "scopt" sha1 "e078455e1a65597146f8608dab3247bf1eb92e6e"
+)
+
+verifyOptions in verify := VerifyOptions(


is this needed here? how does it relate to the options in build.sbt?

This file is auto-generated, see the PR description

LukasGasior1 · 2018-04-18T09:51:59Z

One thing I noticed: we probably don't want to store the importPrivateKey command in the history

rtkaczyk · 2018-04-18T11:14:08Z

importPrivateKey command in the history

That's a really good point!

LukasGasior1 · 2018-04-18T12:02:36Z

Another one: -d flag does not work:

ubuntu@ip-10-0-1-82:~/mantis-faucet/mantis-1.0-daedalus-rc1$ ./bin/mallet -d "/home/ubuntu/keystore" "http://10.0.1.82:8546"                                                                                                               
[residual] arg = '/home/ubuntu/keystore'                                                                                                                                                                                                  
[residual] arg = 'http://10.0.1.82:8546'
# Executing command line:
java
-Dconfig.file=./conf/mantis.conf                                                                                                                                                                                                          
-Dlogback.configurationFile=./conf/logback.xml                                                                                                                                                                                            
-Xss10M
-Dconfig.file=./conf/mallet.conf                                                                                                                                                                                                          
-cp                                                                                                                                                                                                                                       

-jar                                                                                                                                                                                                                                      
/home/ubuntu/mantis-faucet/mantis-1.0-daedalus-rc1/lib/mantis.mantis-1.0-daedalus-rc1-launcher.jar                                                                                                                                        
mallet                                                                                                                                                                                                                                    
/home/ubuntu/keystore
http://10.0.1.82:8546

Error: node URI scheme must be explicit and either 'http' or 'https'                                                                                                                                                                      
Error: Unknown argument 'http://10.0.1.82:8546'                                                                                                                                                                                           
Try --help for more information.

full --data-dir works just fine

rtkaczyk · 2018-04-19T13:02:09Z

@LukasGasior1 -d was conflicting with the launcher's script debug option, but adding -- fixed the issue.

Also on the subject, note there are no longer separate mantis and mantis-app scripts. Turns out that adding -Dconfig.file=... in the mallet script overrides the defaults from application.ini (which was the original problem that the hack tried to solve)

In other news:

importPrivateKey is no longer saved in history
absence of a receipt is correctly handled
receipts are displayed as formatted JSON

LukasGasior1 · 2018-04-21T18:33:15Z

src/main/scala/io/iohk/ethereum/mallet/main/OptionParser.scala

+    }
+
+  def apply(args: Seq[String]): Option[ClOptions] = {
+    val parser = new scopt.OptionParser[ClOptions]("scopt") {


super minor: the string argument here is a programName, guess it should be Constants.AppName rather than "scopt"

Not that minor, good catch

…/mallet

LukasGasior1 · 2018-04-26T09:30:44Z

src/main/scala/io/iohk/ethereum/mallet/service/RpcClient.scala

+  // TODO: CL option to enable akka logging
+  private val akkaConfig = ConfigFactory.load("mallet")
+
+  implicit val actorSystem = ActorSystem("mallet_rpc", akkaConfig)


why not instantiate it in Mallet app and pass to RpcClient as a parameter?

I meant to avoid ActorSystem leaking out of RpcClient cause it's an internal detail. Obviously this is not the case now, because I forgot to add shutdown() method to RpcClient - then the actor system could be private to RpcClient. What do you think about such approach?

To me, actor system is not really internal to rpc client. Even though for mallet it's only used here, it has an impact on the whole app (for example, keeps it alive until it's shutdown), that's why I'd opt for putting in on app-level and pass (perhaps implicitly) to rpc client. It would also make RpcClient's dependencies more clear as well as make it reusable.

also, is there any reason why we're using the actorSystem.dispatcher instead of global EC or, even better, implicit ec passed as a constructor param?

What's wrong with using dispatcher?

since you're using blocking operation (await), doesn't that mean that one of the threads that akka uses internally will be blocked? I know, in this case it doesn't matter since RpcClient is the only component using actors, but for the reasons mentioned above (reusability) I think it would be better to let the user of this class choose which EC he prefers (or, alternatively use global ec that won't block system dispatcher's thread).

LukasGasior1

one small comment, besides that LGMT

loverdos

What I do not see is a small document that introduces mallet and its use-case, presents the design without being elaborate on this, comments on configuration options and also presents a couple of examples. In other words: a tech doc.

Any explanations, comments, etc we do on this PR description and elsewhere are great during the review process but are lost, no-one looks back on PRs and we actually do not own them and cannot reproduce them (they are not in git). What remains is the code and (hopefully) an accompanying, lightweight tech doc for the new functionality. We can just call it the mallet README, if we like.

loverdos · 2018-04-26T08:38:29Z

src/main/scala/io/iohk/ethereum/mallet/main/ConstPasswordReader.scala

+import io.iohk.ethereum.mallet.service.PasswordReader
+
+/**
+  * This implementation of [[PasswordReader]] is in non-interactive mode when the password is


We have a small warning in scaladoc generation:

[warn] mantis/src/main/scala/io/iohk/ethereum/mallet/main/ConstPasswordReader.scala:5: Could not find any member to link for "PasswordReader". [warn] /** [warn] ^ [warn] one warning found

The fix is to use the fully qualified name:

[[io.iohk.ethereum.mallet.service.PasswordReader PasswordReader]]

Isn't it a bug? The PasswordReader is imported. See this scaladoc

@loverdos wdyt?

It works only if the referenced thing is in the same package, not if it is imported. I do not know if this is intended behaviour or a bug though. So I just use the FQN on these occasions.

loverdos · 2018-04-26T08:56:41Z

build.sbt

@@ -186,7 +96,6 @@ scalacOptions := Seq(
  "-unchecked",
  "-deprecation",
  "-feature",
-  "-Xfatal-warnings",
  "-Xlint:unsound-match",


After fixing the scaladoc warning, I re-enabled -Xfatal-warnings and the build went fine from an sbt clean state.

I can re-enable it. I always disable it when working on something complicated, because of unused imports. I'm adding and removing code back and forth, and going to imports for each such change kills my concentration and infuriates me.

There is a keystroke in IntelliJ that takes care of unused imports :)

loverdos · 2018-04-26T08:57:18Z

src/main/resources/mallet.conf

+  ssl-config {
+    trustManager = {
+      stores = [
+        # TODO: move to Wiki maybe?


Yes, we have to make some documentation elsewhere ...

As discussed, it will be done in #445

loverdos · 2018-04-26T10:15:38Z

src/main/scala/io/iohk/ethereum/mallet/service/RpcClient.scala

+
+/**
+  * Talks to a node over HTTP(S) JSON-RPC
+  * Note: the URI schema determins whether HTTP or HTTPS is used


typo: determines

loverdos · 2018-04-26T10:22:28Z

src/main/scala/io/iohk/ethereum/mallet/interpreter/Commands.scala

+      }
+    }
+
+    val helpHeader = "fetch the receipt for a know transaction hash"


typo: known

rtkaczyk · 2018-04-27T09:40:19Z

src/main/scala/io/iohk/ethereum/mallet/service/RpcClient.scala

+    * This factory method is defining an ActorSystem, ActorMaterializer and ExecutionContext for
+    * the [[RpcClient]]. To customize these dependencies use [[RpcClient]]'s constructor
+    */
+  def apply(node: Uri): RpcClient = {


@LukasGasior1 hopefully this should address your remarks about reusability, while limiting the scope of dependencies in mallet.

ok, let's leave it that way

rtkaczyk added 3 commits April 9, 2018 13:40

[CGKIELE-107] mallet - a minimalist wallet

24021b1

[CGKIELE-107] mallet - major refactoring of interpreter mechanics; ht…

545dfa9

…tps support

[CGKIELE-107] mallet - non-interactive mode; packaging

4066cde

rtkaczyk added the don't merge label Apr 9, 2018

rtkaczyk requested review from LukasGasior1 and loverdos April 9, 2018 14:35

loverdos reviewed Apr 11, 2018

View reviewed changes

[CGKIELE-107] mallet - fix sbt-verify

792e0af

rtkaczyk force-pushed the feature/mallet branch from da3349b to 792e0af Compare April 11, 2018 12:48

rtkaczyk added 3 commits April 12, 2018 10:35

[CGKIELE-107] mallet - added code comments (scaladoc)

be26a7e

[CGKIELE-107] mallet - help system for commands

8d1b8b0

[CGKIELE-107] mallet - improved command line options

06ea794

rtkaczyk removed the don't merge label Apr 13, 2018

LukasGasior1 mentioned this pull request Apr 16, 2018

Add faucet #439

Merged

LukasGasior1 reviewed Apr 17, 2018

View reviewed changes

rtkaczyk force-pushed the feature/mallet branch from 8c84cf8 to dbb8155 Compare April 19, 2018 12:54

[CGKIELE-107] mallet - several fixes as per PR review

0494ee4

rtkaczyk force-pushed the feature/mallet branch from dbb8155 to 0494ee4 Compare April 19, 2018 13:02

LukasGasior1 reviewed Apr 21, 2018

View reviewed changes

rtkaczyk added 2 commits April 23, 2018 08:37

[CGKIELE-107] mallet - minor fix: use Constants.AppName

6e2352b

Merge remote-tracking branch 'origin/phase/iele_testnet' into feature…

81f1d70

…/mallet

rtkaczyk mentioned this pull request Apr 24, 2018

[CGKIELE-107] mallet - IELE transactions #445

Merged

1 task

LukasGasior1 reviewed Apr 26, 2018

View reviewed changes

LukasGasior1 approved these changes Apr 26, 2018

View reviewed changes

loverdos reviewed Apr 26, 2018

View reviewed changes

[CGKIELE-107] mallet - PR review fixes

3da9694

rtkaczyk commented Apr 27, 2018

View reviewed changes

rtkaczyk merged commit 3da9694 into phase/iele_testnet Apr 27, 2018

rtkaczyk deleted the feature/mallet branch April 27, 2018 12:11

		@@ -0,0 +1,3 @@
		#!/bin/bash

		./bin/mantis-app -Dconfig.file=./conf/mantis.conf -Dlogback.configurationFile=./conf/logback.xml -J-Xss10M

[CGKIELE-107] mallet #436

[CGKIELE-107] mallet #436

Conversation

rtkaczyk commented Apr 9, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasGasior1 commented Apr 18, 2018

rtkaczyk commented Apr 18, 2018

LukasGasior1 commented Apr 18, 2018

rtkaczyk commented Apr 19, 2018

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

LukasGasior1 left a comment

Choose a reason for hiding this comment

loverdos left a 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

rtkaczyk Apr 26, 2018 • 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

rtkaczyk commented Apr 9, 2018 •

edited

Loading

rtkaczyk Apr 26, 2018 •

edited

Loading