Closed SomberNight closed 1 month ago
Remarks:
1. Perhaps we should have both `wizard_data['wallet_name']` and `wizard_data['wallet_path']`. Searching for existing usages of `wizard_data['wallet_name']` shows that in some cases it contains a full path, and in other cases it contains just the basename.
This is a bit of legacy unfortunately ported over from the old wizard. The most consistent solution IMO is to convert to a fully qualified path immediately and use that everywhere. The bare name is easy to extract in places where that is applicable.
2. Looks like `WalletStorage.__init__` is not a good API: it mixes creating new files and opening existing files. Perhaps we should be explicit and have two separate APIs. That would reveal and prevent similar bugs.
agreed. See also this comment w.r.t. wallet path inconsistencies: https://github.com/spesmilo/electrum/issues/8901#issuecomment-1964470271
On macOS, when using the release binary (4.5.4), and double-clicking the .app to start Electrum, offline 2fa wallet creation fails. To trigger the bug, it is necessary to start Electrum by double-clicking the .app using the desktop environment, instead of using the terminal, as this results in
os.getcwd() == "/"
.In the second stage of the offline 2fa wallet creation (so during the online part), see snippet: https://github.com/spesmilo/electrum/blob/32d5e1724934f7d2976c675d577fde0e632d4855/electrum/gui/qt/wizard/wallet.py#L182-L186
wizard_data['wallet_name']
is the file basename, instead of a full pathWalletStorage(wallet_file)
will start creating a "new file"WalletStorage.__init__
will callstandardize_path
, which callsos.path.abspath()
, which usesos.getcwd()
to convert the filename to an abs pathcwd
, either a hard error, or silent soft error will follow:/
, andtest_read_write_permissions
will error: https://github.com/spesmilo/electrum/blob/32d5e1724934f7d2976c675d577fde0e632d4855/electrum/storage.py#L71WalletStorage.__init__
will succeed as if we were creating a new file, andQENewWalletWizard.is_finalized()
will exit successfully on line 186: https://github.com/spesmilo/electrum/blob/32d5e1724934f7d2976c675d577fde0e632d4855/electrum/gui/qt/wizard/wallet.py#L185-L186related: https://github.com/spesmilo/electrum/issues/8815#issuecomment-2094259186
In either case, the problem is that
WalletStorage(wallet_file)
intends to create a storage for an existing file, but instead, due to confusion with the file paths, it ends up creating a new storage. The following patch illustrates the problem -- the assert will trigger:Remarks:
wizard_data['wallet_name']
andwizard_data['wallet_path']
. Searching for existing usages ofwizard_data['wallet_name']
shows that in some cases it contains a full path, and in other cases it contains just the basename.WalletStorage.__init__
is not a good API: it mixes creating new files and opening existing files. Perhaps we should be explicit and have two separate APIs. That would reveal and prevent similar bugs.