handle exceptions which have no message

[jdlcdl]

Discovered while researching Nico's issue #369,

An exception that is raised and caught by the controller which has no exception message was causing another IndexError while handling the original exception.

File "/home/pi/seedsigner-dev/src/seedsigner/controller.py", line 312, in handle_exception
    exception_msg = last_line.split(":")[1]
IndexError: list index out of range

This pr intends to handle exceptions which have no exception message (as in assert False) so that the program does not crash and so that the user sees a dire warning with exception type and line number.

PLEASE CORRECT ME IF IM WRONG ABOUT THIS ASSUMPTION
... because I could very-well understand the value in wanting the system to go no further in some cases... but I have no evidence that we already do this with expressed intent elsewhere. It's rare that exceptions are raised w/o exception messages, but it is happening, at least, in embit.bip85's derive_entropy(), like assert max(path) < H as it's checking that the child index is not greater than 2**31-1, as Nico reported yesterday.

IF THERE ARE TIMES WHEN WE WANT SEEDSIGNER TO CRASH, whether in our local codebase or as dependencies want to crash, then this pr should NOT be merged. I don't have enough history here to make this call (but thinking exit() would be more obvious than relying on unhandled asserts.)

[newtonick]

I don't know of any scenario in the code that requires/expects a crash. The purpose of handle_exception is to catch unintentional exceptions. It looks like you have PR #372 to handle the specific exception Nico uncovered.

This PR LGTM. ACK tested on the bip85 index > 32 byte int.

[jdlcdl]

The purpose of handle_exception is to catch unintentional exceptions.

This is my thought too, but I've been guilty in the past of conventions like "Never catch generic Exception or failed assert." and then peppering code liberally with asserts... not expecting to ever cross those situations but wanting alarm bells to go off (app is down) if that happens; when it's on a server with logs we can check, it's ok, but if it's on devices we dont own, id rather the user says "I dont understand this Assert screen, but it says file x, line y."

[kdmukai]

@jdlcdl can you add a test case for this in test_controller.py? You should be able to follow the other tests in there to get a Controller instance and then just directly call handle_exception() and pass it various types of manually instantiated Exceptions, etc).

try:
    raise Exception("blah!")
except Exception as e:
    destination = controller.handle_exception(e)
    assert destination.view_args["error"][2] == "blah!"

That basic idea could be wrapped up in a reusable helper function to go through a series of different Exception scenarios (with message, without message, different types of Exceptions.

def process_exception(exception):
    try:
        raise exception
    except Exception as e:
        destination = controller.handle_exception(e)
        return destination.view_args["error"]

error = process_exception(Exception("foo"))
assert ...

error = process_exception(Exception())
assert ...

error = process_exception(KeyError("key blah is not in blah"))
assert ...

[jdlcdl]

@jdlcdl can you add a test case for this in test_controller.py?

I appreciate that you've raised the bar here, challenging me to do better.

It makes me realize that I should have started with the test that proves a "breakage", forcing me to imagine what "working" would look like, and proving that too. In writing the latest commit acc8517, I backed out my changes in order to make sure that the test would raise red flags, and then added the same code (+ a comment) to the controller as were already in commit 59d3787.

The test was not as easy as I thought it would be, the end result a bit different than what you propose above -- but the intent was to do exactly what you are proposing.

[kdmukai]

I LOVE that you explained the format here!

@bitcoinprecept We can drop a super low-priority TODO pin on that error obj to define it as a proper class someday. It's obviously a bit fragile in the current approach (that was my bad design). Would be a good first issue for a beginner.
see:

seedsigner/src/seedsigner/controller.py

Lines 327 to 331 in acc8517

	error = [
	exception_type,
	line_info,
	exception_msg,
	]

[jdlcdl]

Not quite understanding if a conversation should be resolved by:

• the one who proposed the comment/idea
• or the one who claims to have acknowledged and followed it.
  ... I saw a "resolve conversation" button for the first time, and I clicked it... thrice.

[kdmukai]

Not quite understanding if a conversation should be resolved by:

I'm not sure what common practice is, either. Though when I'm the submitter, I tend to leave them as-is for the original reviewer/commenter to resolve. I figure it was their concern/feedback, so they should decide if the update resolves it. For simple changes it doesn't matter at all, but I could definitely imagine cases where the fix doesn't quite fully address the original note.

[newtonick]

LGTM

[newtonick]

ACK Tested