Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

Testing Errors #8

Open
abhiShandy opened this issue Sep 15, 2021 · 18 comments
Open

Testing Errors #8

abhiShandy opened this issue Sep 15, 2021 · 18 comments

Comments

@abhiShandy
Copy link

I get a prompt with Lost connection to backend! Please restart the backend server. even when the backend server is running and receiving API calls from the frontend port.

I don't see the error message on the terminal running the frontend.

@abhishek0405
Copy link
Collaborator

I get a prompt with Lost connection to backend! Please restart the backend server. even when the backend server is running and receiving API calls from the frontend port.

I don't see the error message on the terminal running the frontend.

This aspect seemed to be working fine in the basic tests we ran. Any specific instance where you witnessed this behavior?

@abhiShandy
Copy link
Author

Nothing special. I'm simply trying to create a wallet.

@abhishek0405
Copy link
Collaborator

abhishek0405 commented Sep 16, 2021

Nothing special. I'm simply trying to create a wallet.

abhishek0405/joinmarket-clientserver@89f9571
Check this out, should fix the lost connection alert.

@abhiShandy
Copy link
Author

I pulled the latest commit (f8bb12a4421cd5114d4fdd4d7cba133f316ca118)

When I create a wallet, I don't see the "Lost Connection" error anymore but I get the "Unexpected Error. Please Try again".

The backend throws the following error:

File "jmwalletd.py", line 397, in createwallet
return self.initialize_wallet_service(request, wallet, seedphrase=seedphrase_help_string)
File "jmwalletd.py", line 409, in initialize_wallet_service
encoded_token = jwt.encode({"wallet": "name_of_wallet","exp" :datetime.datetime.utcnow()+datetime.timedelta(minutes=30)},"secret")
builtins.AttributeError: module 'jwt' has no attribute 'encode'

I can see the wallet created successfully when I click on "show wallets" but I'm unable to open/unlock it.

@abhiShandy abhiShandy changed the title Lost connection to backend Testing Errors Sep 16, 2021
@abhishek0405
Copy link
Collaborator

I pulled the latest commit (f8bb12a4421cd5114d4fdd4d7cba133f316ca118)

When I create a wallet, I don't see the "Lost Connection" error anymore but I get the "Unexpected Error. Please Try again".

The backend throws the following error:

File "jmwalletd.py", line 397, in createwallet
return self.initialize_wallet_service(request, wallet, seedphrase=seedphrase_help_string)
File "jmwalletd.py", line 409, in initialize_wallet_service
encoded_token = jwt.encode({"wallet": "name_of_wallet","exp" :datetime.datetime.utcnow()+datetime.timedelta(minutes=30)},"secret")
builtins.AttributeError: module 'jwt' has no attribute 'encode'

I can see the wallet created successfully when I click on "show wallets" but I'm unable to open/unlock it.

Uninstall the jwt module you have right now, and run pip install pyjwt . Will fix this error. (import jwt will remain the same)
Thanks for pointing this out, should update the requirements in the backend once so that it is clear.

@abhiShandy
Copy link
Author

abhiShandy commented Sep 16, 2021

Thanks for solving it so quickly! The "create wallet" step works fine now.

I have found three new issues:

  1. I created a bunch of wallets to test. If I unlock one of them and refresh the page, the lock file is also listed in the wallet list.
  2. On unlocking a particular wallet, clicking "open" button on any wallet opens the unlocked wallet.
  3. If all wallets are locked, and I try to open a wallet, an error page shows up.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 17, 2021

I created a bunch of wallets to test. If I unlock one of them and refresh the page, the lock file is also listed in the wallet list.

I introduced this error yesterday, sorry.
The change I made in abhishek0405/joinmarket-clientserver@f8bb12a was necessary to recognize the right data directory (e.g. if you were on Mac or were doing testing locally you would have the wrong directory), but as you can see in the comment in that commit, there is still an issue arising from:

We never insisted on a specific file extension for joinmarket wallet files, jmdat was simply a convention. People may have wallets with different naming. Until we add code to validate whether a given file is a real JM wallet file, we will have to revert to the "only accept .jmdat files" rule. I will do this now.

AdamISZ referenced this issue in abhishek0405/joinmarket-clientserver Sep 17, 2021
@abhiShandy
Copy link
Author

abhiShandy commented Sep 17, 2021

If we are reading only jmdat files in the "display" wallet page, we should also add code to create wallets with that extension even when the user doesn't specify that.

Moreover, I didn't mean to restrict the filename convention, I just wanted the code to ignore the lock files (like .test.lock) while accepting other file extensions.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 17, 2021

If we are reading only jmdat files in the "display" wallet page, we should also add code to create wallets with that extension even when the user doesn't specify that.

Agreed (though it's a bit incomplete; code must apply also to command line and Qt gui; and people can rename files. Only full solution is as per the TODO comment).

Moreover, I didn't mean to restrict the filename convention, I just wanted the code to ignore the lock files (like .test.lock)

Only not showing .*.lock files seems like a not-quite-right solution to me, what if there is some other file in there. Sometimes people make *.bak files manually for backing things up. Etc.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 17, 2021

About:

2.On unlocking a particular wallet, clicking "open" button on any wallet opens the unlocked wallet.
3. If all wallets are locked, and I try to open a wallet, an error page shows up.

My comment is: I think the method of display is causing the functional issue here, if that makes any sense. I think the logical thing is to show all the *.jmdat filenames in a list, with the user able to select only 1, then at the bottom have just one button 'unlock', leading to password prompt -> display page automatically. Then, a lock button could be in the display page, which would reset the state of the app to its starting page, and you could start again for a new wallet (this is of course related to the fact that our backend does not currently have anything other than one-wallet-at-a-time processing).

@abhishek0405
Copy link
Collaborator

  1. On unlocking a particular wallet, clicking "open" button on any wallet opens the unlocked wallet.
  2. If all wallets are locked, and I try to open a wallet, an error page shows up.

Yes I had noticed this, the behaviour is mainly because we haven't added a state verification in the client side while clicking this open button.I tried it once but was getting some other errors (maybe some bug while implementing,will look into it again). Anyways I feel adding the state verification in our React App should fix this.

@abhishek0405
Copy link
Collaborator

My comment is: I think the method of display is causing the functional issue here, if that makes any sense. I think the logical thing is to show all the *.jmdat filenames in a list, with the user able to select only 1, then at the bottom have just one button 'unlock', leading to password prompt -> display page automatically. Then, a lock button could be in the display page, which would reset the state of the app to its starting page, and you could start again for a new wallet (this is of course related to the fact that our backend does not currently have anything other than one-wallet-at-a-time processing).

This is a viable solution as well, we felt that providing individual controls for each wallet could give a better interface. One slight improvement to the current implementation could be that we highlight the current wallet loaded(different background for example) to make it clearer for users.

@abhiShandy
Copy link
Author

abhiShandy commented Sep 17, 2021

If I start the maker service with no coins, the backend crashes. The frontend should ideally convey the error message (do not have any coins left) and go back to display wallet page.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 20, 2021

If I start the maker service with no coins, the backend crashes. The frontend should ideally convey the error message (do not have any coins left) and go back to display wallet page.

Hi,
As of 7c79f5c there is a meaningful change, part of which addresses that issue.

(You will need commit JoinMarket-Org/joinmarket-clientserver@4ca735d or later from the Python backend code branch JoinMarket-Org/joinmarket-clientserver#996 , else it won't work).

Basically, we now poll (though at some point we will subscribe instead), every second, the backend for the latest state. Now, when you try to start the maker service with an empty wallet it will say "please check the status bar for updates" (and the backend will not crash), and you will see that the "Yg (yield generator)" does not go into a "true" state (while it does, in the case of a funded wallet.

While these updates are imo a good solution for this general issue (making sure the client has an up to date record of what is happening), there are two immediate things to address still:

  • Make the status bar stay on top of the app page always, and look nicer (and later, add more useful information as well as these basics)
  • The general layout of "pick, unlock, show, lock" wallet is definitely not viable and I think must be fixed something like I described above. At least in this commit, we now have a record of which wallet is currently open, but that's just a start.

@abhiShandy
Copy link
Author

abhiShandy commented Sep 20, 2021

Thanks for your commit. The status bar is quite useful. However, something else went wrong. I can't see the accounts/mixdepths once I open the wallet. I just see the "show UTXOs" button which also leads to an error since the wallet is empty..

Edit: I restarted the backend and it fixed the issue.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 20, 2021

Yeah. I think there are situations where when the backend is not restarted, the frontend does not realize anything is wrong. Restarting fixes it but we need better info. There are still a number of corner cases like this, feel free to keep adding any more specific cases you come across, thanks.

@AdamISZ
Copy link
Member

AdamISZ commented Sep 21, 2021

8f26259 moves the status message to text in the navbar so it's always visible; not amazing but a reasonable start.

The big thing now is to have the wallets as a component that can be easily selected from and then "unlock" auto-displays.

After that's done, probably switch the modal dialogs from bare alerts to react-bootstrap style alerts and/or input boxes.

If anyone else wants to work on that let us know. For myself I'd just like to see it in a "reasonably functional" state so that it can work for testing the backend. Professionals can make it actually good, later :)

@AdamISZ
Copy link
Member

AdamISZ commented Sep 30, 2021

See new API docs as per JoinMarket-Org/joinmarket-clientserver@b527bbb ; code here will need to be updated for API changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants