search

JoinMarket-Org / jm-web-client

11 stars 4 forks source link

Testing Errors #8

Open abhiShandy opened 3 years ago

abhiShandy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

abhishek0405 commented 3 years ago

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

https://github.com/abhishek0405/joinmarket-clientserver/commit/89f9571bf46ddcfb1d6298924a4df75da0efc6c8 Check this out, should fix the lost connection alert.

abhiShandy commented 3 years ago

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.

abhishek0405 commented 3 years ago

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 thejwt 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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/abhishek0405/joinmarket-clientserver/commit/f8bb12a4421cd5114d4fdd4d7cba133f316ca118 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.

abhiShandy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

About:

2.On unlocking a particular wallet, clicking "open" button on any wallet opens the unlocked wallet.

  1. 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 commented 3 years ago
  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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/JoinMarket-Org/jm-web-client/commit/7c79f5cd2d457713cce0be1726b93346d8f4b750 there is a meaningful change, part of which addresses that issue.

(You will need commit https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/996/commits/4ca735d49d197b6ca1f695dea6e09f13ddeaea9b or later from the Python backend code branch https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/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:

abhiShandy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

https://github.com/JoinMarket-Org/jm-web-client/commit/8f2625919f62e513b6343cd3d3077047800e5e64 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 commented 3 years ago

See new API docs as per https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/996/commits/b527bbb180df74627fe20dabf7b3ed613230a5f8 ; code here will need to be updated for API changes.