search

monero-ecosystem / monero-python

A comprehensive Python module for handling Monero cryptocurrency
BSD 3-Clause "New" or "Revised" License
247 stars 80 forks source link

Added the functionalities for creating, opening and closing a wallet via RPC #100

Closed 7jw92nVd1kLaq1 closed 3 years ago

7jw92nVd1kLaq1 commented 3 years ago

Modified a bit to add functionalities for:

  1. Creating a wallet via RPC - when launching the RPC with the argument '--wallet-dir', you are able to create a wallet with the function "create_wallet(filename, password='')" of the class Wallet in wallet.py. However, this function requires the class to be initialized with the argument 'wallet_open' set to value False, which prevents Wallet.refresh() from being invoked.

Example: w = Wallet(wallet_open=False) w.create_wallet(filename, password (Optional))

  1. Opening a wallet via RPC - This also requires the same argument as the first one - '--wallet-dir.' You can open a wallet within the same directory you specified in the argument with the function "Wallet(wallet_open=False).open_wallet(filename, password='')"

  2. Closing a wallet via RPC - The function to close the wallet is "close_wallet()." This works with or without '--wallet-dir', since all it needs is a wallet being open in RPC. If, however, one needs to access another wallet file after closing the current wallet, you have to launch the RPC with the '--wallet-dir' argument to access different wallets in the directory.

I just wanted to be able to access multiple wallets within the same directory without relaunching the RPC, which was kind of inconvenience.

emesik commented 3 years ago

I'm quite skeptical about it. This is a very backend-specific solution which gets promoted to the higher level API. Personally I have bad experience with working on wallet dirs. The switch and resync were extremely slow an prone to errors. Perhaps that has changed. What's your experience there?

I'll do a code review as well, as I have some doubts regarding the tests.

7jw92nVd1kLaq1 commented 3 years ago

Here are the bash commands I ran the RPC wallet with:

./monero-wallet-rpc --daemon-address node.moneroworld.com:18089 --disable-rpc-login --wallet-dir . --rpc-bind-port 18088

./monero-wallet-rpc --daemon-address uwillrunanodesoon.moneroworld.com:18089 --disable-rpc-login --wallet-dir . --rpc-bind-port 18088

I tested it with two different remote nodes since I couldn't download the entire blockchain due to a lack of space. The first node I tested with was "node.moneroworld.com:18089," which turned out to be pretty frustrating, with HTTP requests seemingly unable to reach the wallet on a constant basis. Wasn't able to consistently switch between wallets nor create a new wallet, with this error "requests.exceptions.ReadTimeout: HTTPConnectionPool(host='127.0.0.1', port=18088): Read timed out. (read timeout=30)"

So this time, I decided to give it another try with another one, which was "uwillrunanodesoon.moneroworld.com:18089." Fortunately, it was surprisingly smooth with no connections to the RPC wallet seeming to drop. Was able to switch between wallets without the timeout error.

Maybe, it has something to do with the connection to the node which might be a factor when it comes to performance of the RPC wallet? I have yet to try with my own local node to confirm whether what I am saying is right though. Would love to hear your experience after trying this out.

ghost commented 3 years ago

This adds some pretty big binary files under tests/data as well.

7jw92nVd1kLaq1 commented 3 years ago

I used the "tests/data" directory for the argument "--wallet-dir," which resulted in the creation of new wallet files under the directory. I can delete the wallet files in the directory if you guys find it redundant.

emesik commented 3 years ago

Are those tests able to be run offline? Or they require a prepared environment?

I see no mocking there, so I guess both monero-wallet-rpc and some local or remote daemon should be running to make the test pass. Besides that, the results of the tests wouldn't depend on Python code only and would reflect the setup, network and monero project condition as well. Quite broad spectrum of possible error sources.

From my own experience, without a local daemon any attempt to work with wallet dirs is going to fail. Remote daemons are too... remote. When they're busy, everything goes down. But even with local one nothing seemed stable enough to run a business on top of it.

Monero project is very permissive with accepting new features and some of them aren't maintained further on. I think the wallet dir idea is one of neglected paths in the development. Nobody pushed to make it work well but also nobody volunteered to cut that branch off.

What I'm concerned the most about, is a situation when we include it in the project and make someone depend on it. We could have been bombarded with bug reports and requests we wouldn't be able to properly respond to.

7jw92nVd1kLaq1 commented 3 years ago

Correct. It requires the wallet daemon to be run in a certain way in order to make it pass the test, so the result of it largely depends on the connection to a node, which can be sporadic at times. Wish it was less buggy on the node part. I guess I am closing this since the feature seems pretty unstable for the most part.