* Add keyboard shortcut for trade process refresh, fix#3905
Trades have been getting stuck in the `Wait for payment` state, perhaps
due to lost mailbox messages, but it's hard to know for sure. There is
currently no way to get out of this state except going to mediation.
With ctrl+R the seller can ask the buyer to refresh the current trade
state and the buyer will resend the
`CounterCurrencyTransferStartedMessage` if they are in the phase
FIAT_SENT.
* Disallow more than one trade refresh per day
* Add refresh button for seller step 2, fix#3905
A seller can ask to refresh the trade process once every 24 hours. This
step has been a problem causing a lot of mediation lately so this is a
way to ask the buyer to resend the CounterCurrencyTransferStartedMessage
This fixes the problem when a mailbox message was lost. To test the
seller need to not get the first CounterCurrencyTransferStartedMessage
sent by the buyer, for example by letting the seednode drop it instead
of sending to the seller. When button is pressed
- a RefreshTradeStateRequest is sent from seller to buyer
- the buyer receives the RefreshTradeStateRequest and
- ignores it if it's not in FIAT_SENT phase
- responds with a CounterCurrencyTransferStartedMessage if in FIAT_SENT
phase and has already sent a CounterCurrencyTransferStartedMessage
* Fix codacy remarks
Move incoming message handling method to the right section
* Add refresh button info text
Hide refresh section when not available rather than graying out
Added info text:
Sometimes P2P network messages acknowledging payment are not delivered,
causing trades to get stuck. Hit the button below to make your peer
resend the last message.
* Fix codacy issues
Replace tail recursion of the play() method with an ordinary loop, to
prevent a new open JAR resource InputStream + sound file OutputStream
(which were created every 4 minute playback) from accumulating on the
stack, closing them inside the loop instead. (This also prevents
eventual stack overflow.)
Also tidy up FileUtil.resourceToFile and put the JAR URL InputStream in
a try-with-resources block, to ensure that it doesn't leak either.
There is currently no explicit rule for how option-related elements are
ordered in the code, but it is important to understand that the order in
which options are registered via parser.accept() calls is the order in
which they appear in --help output.
It would be nice to group options together into sections and separate
them in the --help output with section headers similar to the way that
Bitcoin Core's help output does it, but this is not a built-in option
with the JOpt Simple library, and not worth trying to hack into place at
the moment.
Previously ConfigTests constructed Config instances with string-based
options, e.g.:
Config config = new Config("--appName=My-Bisq");
The advantage here is clarity, but the downside is repetition of the
option names without any reference to their corresponding Config.*
constants.
One solution to the problem would be to format the option strings using
constants declared in the Config class, e.g.:
Config config = new Config(format("--%s=My-Bisq", APP_NAME));
but this is verbose, cumbersome to read and write and requires repeating
he '--' and '=' option syntax.
This commit introduces the Opt class and the opt() and configWithOpts()
methods to ConfigTests to make testing easier while using constant
references and preserving readability. e.g.:
Config config = configWithOpts(opt(APP_NAME, "My-Bisq"));
In the process of making these changes a bug was discovered in the
monitor submodule's P2PNetworkLoad class and that has been fixed here as
well.
This change also required introducing several option name constants that
had not previously been extracted in order to be referenced within
ConfigTests. For consistency and completeness, all additional option
names that did not previously have a contstant now have one.
Previously (as of the prior commit), a warning was issued if a
non-default config file path was specified at the command line, and then
the default config file path was used as a fallback. On review, however,
it would be better to halt execution immediately if the config file does
not exist. There is no risk of breaking backward compatibility by doing
this as Bisq never had a --configFile option before the recent commits
that introduce it. Furthermore, there is no clear benefit to the
fallback approach. If the user specifies a given config file and it does
not exist, they may not see the warning message in the log, and they may
be left with the impression that they are running against their custom
config file when in fact they are running against the default (which may
be empty or non-existent itself). Thus throwing an exception as is now
done in this commit should make everything more explicit and clear.
This behavior had already been implemented prior to this commit, but has
now been tested and improved with refactoring and logging messages.
Note that this approach emulates Bitcoin Core's own behavior. When
running, for example, `bitcoind -conf=rel/path/to/bitcoin.conf`, the
relative path is prefixed / fully qualified by the value of the
`datadir` option. So if `datadir` equals `~/Library/Application
Support/Bitcoin`, then the `conf` option value above would be fully
qualified as
~/Library/Application Support/Bitcoin/rel/path/to/bitcoin.conf
If the argument to `-conf` is an absolute path, e.g.
`/tmp/bitcoin.conf`, then that absolute path is used without further
modification or qualification. It is assumed that the rationale for this
behavior is to avoid accidentally running against the wrong conf file
because `bitcoind` was invoked in a different directory than usual or
because a malicious actor replaced the relative conf file with their own
version.
Bisq's new `--configFile` option works (and is now tested to work) in
the same way: relative paths get prefixed by the value of
Config.getAppDataDir(), and absolute paths are processed unmodified.
This new class encapsulates all functionality related to detecting a
local Bitcoin node and reporting whether or not it was detected.
Previously this functionality was spread across the Config class
(formerly BisqEnvironment) with its mutable static
isLocalBitcoinNodeRunning property and the BisqSetup class with its
checkIfLocalHostNodeIsRunning method. All of this functionality now
lives within the LocalBitcoinNode class, an instance of which is wired
up via Guice and injected wherever necessary.
Note that the code for detecting whether the node is running has been
simplified, in that it is no longer wrapped in its own dedicated Thread.
There appears to be no performance benefit from doing so, and leaving it
in place would have made testing more difficult than necessary.
Several methods in BisqSetup have also been refactored to accept
callbacks indicating which step should be run next. This has the effect
of clarifying when the step2()-step5() methods will be called.
Previously this static property had been managed within
BaseCurrencyNetwork itself and was accessed directly by callers. Now it
is managed within Config, made private and accessed only via the
new and well-documented baseCurrencyNetwork() method. The same goes for
baseCurrencyNetworkParameters().
It is unfortunate that we must retain these mutable static fields and
accessors, but after trying to eliminate them entirely, this approach is
the lesser of two evils; attempting to use a Config instance and
instance methods only ends up being quite cumbersome to implement,
requiring Config to be injected into many more classes than it currently
is. Getting access to the BaseCurrencyNetwork is basically a special
case, and treating it specially as a static field is in the end the most
pragmatic approach.
Prior to this commit, the way that the appDataDir and its subdirectories
were created was a haphazard process that worked but in a fragile and
non-obvious way. When Config was instantiated, an attempt to call
btcNetworkDir.mkdir() was made, but if appDataDir did not already exist,
this call would always fail because mkdir() does not create parent
directories. This problem was never detected, though, because the
KeyStorage class happened to call mkdirs() on its 'keys' subdirectory,
which, because of the plural mkdirs() call ended up creating the whole
${appDataDir}/${btcNetworkDir}/keys hierarchy. Other btcNetworkDir
subdirectories such as tor/ and db/ then benefited from the hierarchy
already existing when they attempted to call mkdir() for their own dirs.
So the whole arrangement worked only because KeyStorage happened to make
a mkdirs() call and because that code in KeyStorage happened to get
invoked before the code that managed the other subdirectories.
This change ensures that appDataDir and all its subdirectories are
created up front, such that they are guaranteed to exist by the time
they are injected into Storage, KeyStorage, WalletsSetup and TorSetup.
The hierarchy is unchanged, structured as it always has been:
${appDataDir}
└── btc_mainnet
├── db
├── keys
├── wallet
└── tor
Note that the tor/ subdirectory actually gets deleted and re-created
within the TorSetup infrastructure regardless of whether the directory
exists beforehand.
Option handling is now the responsibility of the Config class. JOpt's
OptionParser is no longer passed down to BisqExecutable subclasses'
doExecute method, as they can now rely on the Config abstraction.
In previous commits, BisqEnvironment functionality has been fully ported
to the new, simpler and more type-safe Config class. This change removes
BisqEnvironment and all dependencies on the Spring Framework Environment
interface that it implements.
The one exception is the pricenode module, which is separate and apart
from the rest of the codebase in that it is a standalone, Spring-based
HTTP service.
Note that this change makes the user-facing change of renaming
the 'numConnectionForBtc' (singular 'Connection') to
'numConnectionsForBtc' (plural 'Connections'). It is presumed that not
many users are relying on this option for day-to-day operations, and the
singular version was pretty clearly a typo / oversight.
