-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changing BTC network does not have any effect #2048
Comments
I have been able to track the issue down to 83e1dd3. (and probably the issue, where the appdir uses the default dir as well). The issuebisq/core/src/main/java/bisq/core/app/BisqExecutable.java Lines 478 to 482 in 83e1dd3
--baseCurrencyNetwork=BTC_TESTNET )
However, due to bisq/core/src/main/java/bisq/core/app/BisqEnvironment.java Lines 338 to 340 in 83e1dd3
Bisq first parses the commandLineProperties (which already provides the default values due to the abovementioned changes), and thus, the getProperty-call is satisfied already and the getAppDirProperties() are never asked.
This all boils down to a mostly useless FixNow, it is not trivial to change the behaviour while keeping the (most welcome) changes of 83e1dd3.
all in all, I do not want to decide on that alone: @cbeams, @ManfredKarrer any ideas? other
|
Ah great you found out the reason! Same issue was with log path which was now always 'Bisq'. I don't have strong opinions and its not my field of expertise. Leave it to @cbeams to suggest how we should resolve it, but would be good to resolve it soon as people cannot switch to testnet in the UI and it might affect other options as well. |
I stumbled across this when I was playing with 0.9.0, but I hadn't had enough time to fully determine if I was doing something wrong or if there actually was a problem; I'm using the Windows client. One thing that may be relevant to bring up: I tried changing the field in the config file (I don't have access to my PC at this moment to specify exactly what the config file name was), and it reverted back to MAINNET after starting the client. Initially I tried this when the client was running, then restarted. I then tried it after shutting it down, and it also changed the contents of the file on startup. |
This change fixes bisq-network#2048 by removing the assignment of a default value for the `baseCurrencyNetwork` option at the level of the command line option parser. The assignment of this default was an oversight in bisq-network#1961 (specifically commit 83e1dd3) that did not account for the fact that users can change the `baseCurrencyNetwork` value via the Settings screen in the application. When users change the setting in the application, the new value is persisted to <appDataDir>/bisq.properties, which is handled at runtime as a PropertySource with lower precedence than the command line property source, which means that the changed value is never picked up because the higher-precedence command line PropertySource always has a default value. This fix is surgical in that it addresses only this specific option. A subsequent change should address the more general issue that setting defaults in the command line option parser always precludes the possibility of overriding them in bisq.properties. Basically, we should revert to the previous strategy of reporting what the default value will be in the help text without actually assigning a default value in the option parser using the `defaultsTo` method.
Thanks, @freimair for the careful analysis above. Please see #2063, where I've fixed this specific problem, and you'll see in the comment there where I suggest a larger and more general fix. I have a number of additional changes to our option handling infrastructure queued up locally that I have not yet submitted as a pull request. I'll see about working the more general fix into those changes before I submit it. |
If user changes the BTC network in settings and restarts MAINNET will be selected again even if the user has changed to testnet. There is probably some bug in the option parsing. @freimair will look into it.
For now users who want to try out the testnet DAO need to add a program argument:
'--baseCurrencyNetwork=BTC_TESTNET ' to enable testnet.
The text was updated successfully, but these errors were encountered: