Added AcctThread.py utility: #1

scottschurr · 2018-08-14T23:18:15Z

AcctThread.py is a command line utility that takes a ripple account ID as an argument then walks the PreviousTxnID thread left behind in the account root. This walk will, if enough history is available, pass through all transactions affecting the account root in question until we arrive at the creation of the account root.

Output is JSON (with text separators and text comments) to stdout. To capture the output to a file redirect the output.

AcctThread is a command line utility that takes a ripple account ID as an argument then walks the PreviousTxnID thread left behind in the account root. This walk will, if enough history is available, pass through all transactions affecting the account root in question until we arrive at the creation of the account root. Output is JSON (with text separators and text comments) to stdout. To capture the output to a file redirect the output.

seelabs

Certainlly not a python expert, but I did a quick pass.

seelabs · 2018-08-15T01:18:49Z

AcctThread.py

@@ -0,0 +1,280 @@
+#!/usr/local/bin/python3


#!/usr/bin/env python3 is better. python3 doesn't always live in /usr/local/bin

seelabs · 2018-08-15T01:24:48Z

AcctThread.py

+from websocket import create_connection
+
+# Extract command line arguments.
+def extractArgs ():


Don't put a space between a function and the argument parens. The standard for formatting python code is pep8. See this section: https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements

I recommend using an automatic formatter for python code. I use yapf (https://github.com/google/yapf) but there are other good formatters out there as well.

seelabs · 2018-08-15T01:28:28Z

AcctThread.py

+    connectTo = "ws://s2.ripple.com:443"
+
+    usage = 'usage: PyAcctThread <Account ID> [ws://<Server>:<port>]\n'\
+        'If <server>:<port> are omitted defaults to "{0}"'.format (connectTo)


Python 3.6 introduced f strings (see https://www.python.org/dev/peps/pep-0498/). I strongly prefer them for my own scripts, but of course I don't have to worry about supporting multiple python interperters. Your call if you want to support python 3.5 or earlier.

seelabs · 2018-08-15T01:35:01Z

AcctThread.py

+
+from websocket import create_connection
+
+# Extract command line arguments.


Instead of a comment at the top of a function, this should be a docstring. I.e.

def extractArgs(): '''Extrace command line arguments'''

seelabs · 2018-08-15T01:45:17Z

AcctThread.py

+def wsId ():
+    global wsIdValue
+    wsIdValue += 1
+    return wsIdValue


Instead of making wsIdValue global I'd make it an attribute of the function. I.e

def wsId(): wsId.value += 1 return wsId.value wsId.value = 0

seelabs · 2018-08-15T01:48:04Z

AcctThread.py

+        msg = ws.recv ()
+        jsonMsg = json.loads (msg)
+        gotId = jsonMsg["id"]
+        if (gotId == id):


Python if statements don't need parens.

seelabs

I wouldn't put this script in the root directory. We should find or create a subdirectory for it.

seelabs · 2018-08-15T11:46:36Z

AcctThread.py

+def extractArgs ():
+
+    # Default websocket connection if none provided.
+    connectTo = "ws://s2.ripple.com:443"


function and variable names should be snake case. see: https://www.python.org/dev/peps/pep-0008/#function-and-variable-names

seelabs · 2018-08-15T11:49:44Z

AcctThread.py

+
+
+# Used to track websocket requests.
+wsIdValue = 0


global variable should be upper case separated by underscores. I.e WS_ID_VALUE, though see the comment about making this a attribute of the function rather than a global.

seelabs · 2018-08-15T11:50:57Z

AcctThread.py

+
+# Get the websocket response that matches the id of the request.  All
+# responses are in json, so return json.
+def getResponse (ws, id):


id is a builtin. It's better not to use it as a parameter name.

seelabs · 2018-08-15T12:05:11Z

AcctThread.py

+
+if __name__ == "__main__":
+    # Get command line arguments.
+    connectTo, accountId = extractArgs ()


These are global variables. Line 34 has a local variable that shadows this global. There's a couple ways to fix this:

If you want to keep them global, rename them to use the global convention of UPPER_CASE

Create a function main make the variables local

Call the function directly with the tuple returned from extractArgs. I.e. openConnection(*extractArgs())

seelabs · 2018-08-15T12:33:33Z

AcctThread.py

+            return ""
+
+        err = jsonMsg["error"]
+        if jsonMsg[error_message]:


This looks wrong. Did you mean 'error_message'? (and below)

seelabs · 2018-08-15T12:39:27Z

AcctThread.py

+    idLen = len (accountId)
+    if (accountId[:1] != "r") or (idLen < 25) or (idLen > 35):
+        print ('Invalid format for account ID.\n'\
+            'Should start with "r" with length between 25 and 35 characters.\n')


Instead of a line continuation here (and other print statements), I'd pass multiple arguments into print. I.e.

print('long_line_1', 'print_long_line_2')

seelabs · 2018-08-15T12:43:08Z

AcctThread.py

+        # the server does not have enough history.
+        if jsonMsg["error"] == "txnNotFound":
+            print ("Transaction not found.  "
+                "Does your server have enough history?  Unexpected stop.")


I'd single space between sentences

seelabs · 2018-08-15T12:55:39Z

Here's a commit with all my feedback in one place (note, I'm unable to test this locally right now; I have a package conflict between websock and vega, so I can't install websocket): seelabs@ee51bc9

scottschurr · 2018-08-15T19:15:42Z

@seelabs, thanks for all the help. I cherry-picked your feedback and made a few other changes I hope are improvements.

seelabs · 2018-08-15T19:24:13Z

@scottschurr I was more thinking about a "scripts" directory or somesuch to put these standalone scripts. Other than that LGTM. 👍 (and still 👍 if you want to keep the script in the dir you already put it in).

wilsonianb

It works 🎉

wilsonianb · 2018-08-16T16:05:21Z

scripts/AcctThread.py

+
+
+def print_tx(ws, txn_id):
+    '''Request tx by tnx_id, print it, and return the tx as JSON'''


tnx_id -> txn_id

wilsonianb · 2018-08-16T16:10:41Z

scripts/AcctThread.py

+    json_msg = get_response(ws, wsid)
+
+    # Remove the websocket id to reduce noise.
+    json_msg.pop("id", None)


Could "id" be removed inside get_response?
We seem to never want it, and get_response already checks that "id" matches req_id/wsid

wilsonianb · 2018-08-16T16:33:11Z

scripts/AcctThread.py

+          this account.
+       3. Call tx with that TxId.  Save that transaction.
+       4. The tx response should contain the AccountRoot for the account_id.
+          Extract the new value of PreviousTxID from the AccountRoot.


PreviousTxID -> PreviousTxnID
here and in 5 below

wilsonianb · 2018-08-16T16:51:36Z

scripts/AcctThread.py

+
+        err = json_msg["error"]
+        if json_msg['error_message']:
+            err = json_msg['error_message']


I think this could be

err = json_msg['error_message'] or json_msg["error"]

Yup, that would work if my original code were correct. Which it's not. I had failed to check that error handling path. The old code did not work because a missing key in a Dictionary throws a KeyError exception rather than return None. Fortunately your nudge led me to actually test that error condition. So now I've changed the code to this:

if "error_message" in json_msg: err = json_msg["error_message"] else: err = json_msg["error"] + "."

I think that's easier to read than the old code anyway. And I also tested both legs this time 🙂.

wilsonianb · 2018-08-16T16:57:40Z

scripts/AcctThread.py

+        except KeyError:
+            pass  # If the field is not found that's okay.
+
+        # Else look for the next transaction.


You could check for the next transaction first, so that we only have to do the "Else" (check for account creation) once per account.

I think you have a fair point that we'll handle slightly fewer exceptions if we check for the "ModifiedNode" case first, since that's more common. But any node that doesn't meet either of the criteria will receive both checks (and throw both exceptions).

Unfortunately I didn't find an attractive way to look for the presence of nested dictionary entries that doesn't involve KeyError exceptions. And since this is not performance critical code I decided readability was more important than avoiding the exceptions.

wilsonianb · 2018-08-16T17:08:27Z

scripts/AcctThread.py

+import sys
+import websocket
+
+from websocket import create_connection


It'd be helpful to note somewhere that this script needs websocket-client installed

wilsonianb · 2018-08-16T17:13:04Z

scripts/AcctThread.py

+        connect_to = sys.argv[2]
+
+    # Validate the connect_to.
+    if connect_to[:5] != "ws://":


Why not allow wss:// (and use that for the default)?

Sure. That seems to work. When I was first cobbling this together I was having problems with wss:// so I switch to ws:// which worked. But I'm not seeing any issues at the moment, so supporting both seems fine.

scottschurr

Thanks for the excellent comments, @wilsonianb!

scottschurr · 2018-08-16T23:17:20Z

scripts/AcctThread.py

+        except KeyError:
+            pass  # If the field is not found that's okay.
+
+        # Else look for the next transaction.


I think you have a fair point that we'll handle slightly fewer exceptions if we check for the "ModifiedNode" case first, since that's more common. But any node that doesn't meet either of the criteria will receive both checks (and throw both exceptions).

Unfortunately I didn't find an attractive way to look for the presence of nested dictionary entries that doesn't involve KeyError exceptions. And since this is not performance critical code I decided readability was more important than avoiding the exceptions.

scottschurr · 2018-08-16T23:57:40Z

scripts/AcctThread.py

+        connect_to = sys.argv[2]
+
+    # Validate the connect_to.
+    if connect_to[:5] != "ws://":


Sure. That seems to work. When I was first cobbling this together I was having problems with wss:// so I switch to ws:// which worked. But I'm not seeing any issues at the moment, so supporting both seems fine.

scottschurr · 2018-08-17T00:09:17Z

scripts/AcctThread.py

+
+        err = json_msg["error"]
+        if json_msg['error_message']:
+            err = json_msg['error_message']


Yup, that would work if my original code were correct. Which it's not. I had failed to check that error handling path. The old code did not work because a missing key in a Dictionary throws a KeyError exception rather than return None. Fortunately your nudge led me to actually test that error condition. So now I've changed the code to this:

if "error_message" in json_msg: err = json_msg["error_message"] else: err = json_msg["error"] + "."

I think that's easier to read than the old code anyway. And I also tested both legs this time 🙂.

wilsonianb

LGTM 👍
Thanks for being a "First-time contributor" 😄

seelabs reviewed Aug 15, 2018

View reviewed changes

seelabs and others added 3 commits August 15, 2018 10:39

[fold] Feedback from review

45a7229

[FOLD] Commands are dictionaries, improve error responses

8ba1d45

[FOLD] Move AcctThread.py to a subdirectory

533261a

[FOLD] Create scripts subdirectory

ae5ad58

wilsonianb reviewed Aug 16, 2018

View reviewed changes

scottschurr commented Aug 17, 2018

View reviewed changes

[FOLD] More improvements from review

8e39f13

wilsonianb approved these changes Aug 17, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added AcctThread.py utility: #1

Added AcctThread.py utility: #1

scottschurr commented Aug 14, 2018

seelabs left a comment

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs left a comment

seelabs Aug 15, 2018 •

edited

Loading

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018

seelabs Aug 15, 2018 •

edited

Loading

seelabs Aug 15, 2018

seelabs commented Aug 15, 2018

scottschurr commented Aug 15, 2018

seelabs commented Aug 15, 2018

wilsonianb left a comment

wilsonianb Aug 16, 2018

wilsonianb Aug 16, 2018

wilsonianb Aug 16, 2018

wilsonianb Aug 16, 2018

scottschurr Aug 17, 2018

wilsonianb Aug 16, 2018

scottschurr Aug 16, 2018

wilsonianb Aug 16, 2018

wilsonianb Aug 16, 2018

scottschurr Aug 16, 2018

scottschurr left a comment

scottschurr Aug 16, 2018

scottschurr Aug 16, 2018

scottschurr Aug 17, 2018

wilsonianb left a comment


		from websocket import create_connection

		# Extract command line arguments.



		def print_tx(ws, txn_id):
		'''Request tx by tnx_id, print it, and return the tx as JSON'''

Added AcctThread.py utility: #1

Are you sure you want to change the base?

Added AcctThread.py utility: #1

Conversation

scottschurr commented Aug 14, 2018

seelabs 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

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

seelabs Aug 15, 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

seelabs Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Aug 15, 2018

scottschurr commented Aug 15, 2018

seelabs commented Aug 15, 2018

wilsonianb 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

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

scottschurr 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

wilsonianb left a comment

Choose a reason for hiding this comment

seelabs Aug 15, 2018 •

edited

Loading

seelabs Aug 15, 2018 •

edited

Loading