New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccountManager error handling #1195

Closed
palango opened this Issue Jan 5, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@palango
Collaborator

palango commented Jan 5, 2018

Problem Definition

Currently the AccountManager displays the exception text in case something goes wrong with loading a keyfile. Some errors like Expecting value: line 1 column 1 (char 0) are not helpful for the enduser and we should try to show more meaningful messages for common problems.

Solution

Show more meaningful messages for common problems.

Tasklist

  • Investigate common problems like permission problems or malformed json and the exceptions
  • Handle the exception and show nice errors to the user
  • Add a changelog entry.
@vporta

This comment has been minimized.

vporta commented Jan 20, 2018

Can I work on this issue?

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Jan 20, 2018

hey @vporta sure! But I guess we would also need to define the issue a bit better. The error for example that @palango mentions Expecting value: line 1 column 1 (char 0) is most probably due to the loaded account file having invalid json, so the proper error to display to the user would be that the account file is in an invalid format or corrupt.

@vporta

This comment has been minimized.

vporta commented Jan 20, 2018

hey @LefterisJP -- awesome. Should I wait for you to provide a more detailed description or use the one you just provided to me? Should I only be looking in the AccountManager class?

@hackaugusto

This comment has been minimized.

Collaborator

hackaugusto commented Jan 20, 2018

Hey @vporta , thanks for the interest on helping.

Yes, AccountManager is the piece of code that needs extra handling for the error messages, changing the message and fixing this test should be good enough to go:

def test_account_manager_invalid_files(test_keystore, caplog):
with caplog.at_level(logging.DEBUG):
AccountManager(test_keystore)
for msg, file_name, reason in [
('Invalid account file', KEYFILE_INVALID, 'Expecting value: line 1 column 1 (char 0)'),
('Can not read account file', KEYFILE_INACCESSIBLE, 'Permission denied')
]:
for record in caplog.records:
message = record.getMessage()
if msg in message and file_name in message and reason in message:
break
else:
assert False, "'{}' not in log messages".format(msg)

@LefterisJP LefterisJP added this to the Red Eyes milestone Apr 11, 2018

@gitcoinbot

This comment has been minimized.

gitcoinbot commented Apr 11, 2018

This issue now has a funding of 0.12 ETH (50.48 USD @ $420.64/ETH) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $2798.79 more Funded OSS Work Available at: https://gitcoin.co/explorer
@gcarq

This comment has been minimized.

Contributor

gcarq commented Apr 12, 2018

I would take a look if no one is working on this yet

@gitcoinbot

This comment has been minimized.

gitcoinbot commented Apr 12, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

  1. @gcarq

has committed to working on this project to be completed 10 months, 3 weeks from now.

@vs77bb, please see the below comments / questions regarding approach for this ticket from the bounty hunter(s):

@gitcoinbot

This comment has been minimized.

gitcoinbot commented Apr 12, 2018

Work for 0.12 ETH (55.54 USD @ $462.84/ETH) has been submitted by:

  1. @gcarq

Submitters, please leave a comment to let the funder (@vs77bb) (and the other parties involved) that you've submitted you work. If you don't leave a comment, the funder may expire your submission at their discretion.

@vs77bb

This comment has been minimized.

vs77bb commented Apr 28, 2018

@LefterisJP @gcarq This one looks like it has a bit more WIP scope... let me know whenever it's good to pay out!

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Apr 28, 2018

Whenever the issue is closed I guess. The PR is still open and waiting for some work from @gcarq #1355

@gitcoinbot

This comment has been minimized.

gitcoinbot commented May 19, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 0.12 ETH (83.66 USD @ $697.13/ETH) attached to this issue has been approved & issued to @gcarq.

@LefterisJP LefterisJP closed this May 20, 2018

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented May 20, 2018

Closed the issue since the PR that fixed it was merged quite some time ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment