Validate BSQ fee payment using DAO tx info. #6425
Conversation
|
Why do we need the explorer lookup then? We know if the fee payment was BTC or BSQ, at least for maker so we could use the exclusively the DAO data. |
|
I went with the simplest minimal change. Yes, the fee validation could probably be refactored as you suggest. |
I think it would be better to reduce the code to that whats needed now. |
ghost
marked this pull request as draft
ghost
closed this
ghost
reopened this
Previously the BSQ fee payment was determined by parsing a raw tx without relying on the DAO. Unfortunately this turned out to be problematic, so with this change the BSQ fee paid is obtained from the DAO tx, as originally preferred by chimp1984. Unconfirmed transactions will not be able to have their BSQ fees checked so early requests for validation will skip the fee check. This is not a problem since maker fee validation is done by the taker and in the majority of cases will be already confirmed; taker fee validation is done after the first confirm at trade step 2.
ghost
changed the title
|
Refactored as suggested above. Rebased to latest master.
Marked as [DO NOT MERGE] for now. |
ghost
marked this pull request as ready for review
ghost
changed the title
|
Hi would be good to get this merged soon. I have a user in support with multiple offers that are going offline. Shared the logs with @jmacxx who informed me that this PR will fix their issue. |
|
@HenrikJannsen Could you ACK this? |
| private final List<String> errorList; | ||
| private final String txId; | ||
| private Coin amount; | ||
| @Nullable | ||
| private Boolean isFeeCurrencyBtc = null; | ||
| private Boolean isFeeCurrencyBtc = true; |
There was a problem hiding this comment.
As isFeeCurrencyBtc is only used by one constructor and otherwise is not set (and not used) I think keeping it a nullable Boolean is better. In the use cases where its accessed the constructor is use with a non null value, so no need to initialize it with true.
There was a problem hiding this comment.
There is one unused constructor as well. Better to remove that.
| @@ -85,13 +85,13 @@ public TxValidator(DaoStateService daoStateService, | |||
| String txId, | |||
| Coin amount, | |||
| @Nullable Boolean isFeeCurrencyBtc, | |||
There was a problem hiding this comment.
Better remove the @nullable and use primitive boolean value as the callers always provide a non null value.
| validateOfferMakerTx(mempoolRequest, txValidator, resultHandler); | ||
| } else { | ||
| // using BSQ for fees | ||
| UserThread.runAfter(() -> resultHandler.accept(txValidator.validateBsqFeeTx(true)), 1); |
There was a problem hiding this comment.
This BSQ fee check should be gated by a call to isServiceSupported()
There was a problem hiding this comment.
We don't need it there. The MempoolService is not used to validate the BSQ fee because txValidator.validateBsqFeeTx(true) gets the transaction from the DaoStateService.
There was a problem hiding this comment.
With the change to have BSQ fees checked using the DAO database rather than with data obtained from mempool.space, MempoolService is incorrectly named. (In fact, it was unfortunate that it was named referring to an implementation feature rather than functionality).
It would be more appropriately called TradeFeeCheckingService. Fee checking should be gated by isServiceSupported(), so that the filter and config settings can do their job in enabling/disabling the fee checking feature.
ghost
mentioned this pull request
Fixes an issue with BSQ fee validation which has been prompting users to open mediation tickets, as reported by @pazza83
Previously the BSQ fee payment was determined by parsing a raw tx without relying on the DAO. Unfortunately this turned out to be problematic, so with this change the BSQ fee paid is obtained from the DAO tx, as originally preferred by @chimp1984.
Unconfirmed transactions will not be able to have their BSQ fees checked so early requests for validation will skip the fee check. This is not a problem since maker fee validation is done by takers and in the majority of cases will be already confirmed; taker fee validation is done after the first confirm at trade step 2.