search

bisq-network / compensation

@bisq-network contributor compensation requests
https://github.com/bisq-network/docs/tree/master/dao/phase-zero#how-to-request-compensation
20 stars 16 forks source link

For Cycle 56 #1489

Closed alvasw closed 6 months ago

alvasw commented 7 months ago

Summary

Specify the total amount of BSQ you are requesting, along with the USD total and BSQ/USD rate (don't include the brackets!):

Contributions delivered

Add contributions you have delivered and roles you have performed here as new rows in the table below. Role line-items should include an asterisk (*) in the team column.

Title Team USD Link Notes
bisq: PR Review bisq-network/bisq#6986 dev https://github.com/bisq-network/bisq/pull/6986

Nit

Hi yonson2023,
I tested this PR again and it's doesn't include the code review changes, you applied to your previous MoneyBeam PR. Please apply those here too.

---------

ACK

Perfect! Thank you so much!

Test Setup

  • Created Pix and Wise accounts on v1.9.14 on Alice's and Bob's Bisq instance
  • Created Pix and Wise accounts on this PR on Alice's and Bob's Bisq instance

Trades (For Pix and Wise)

  • Between old accounts
    • Checked trade details screen
    • Tested copy button
  • Between new accounts
    • Checked trade details screen
    • Tested copy button
  • Between old and new accounts
    • Checked trade details screen
    • Tested copy button
bisq: PR Review bisq-network/bisq#7007 dev https://github.com/bisq-network/bisq/pull/7007

ACK
bisq: PR Review bisq-network/bisq#7025 dev https://github.com/bisq-network/bisq/pull/7025

ACK
bisq: PR Review bisq-network/bisq#7005 dev https://github.com/bisq-network/bisq/pull/7005

Nit

The QR Code works! I received video recordings of users testing four SEPA banking apps.

Feedback

  • We can show the QR Code by default because most SEPA banking apps support QR Codes.
  • After scanning the QR Code users can't double check the data because it's hidden. I think we should show the BIC and IBAN while showing the QR Code (instead of hiding it).
  • Two of the tested banking apps sent the image to the banking servers to read the QR Code. In both images the text "[...] trade ID or other text like 'bitcoin', 'BTC', or 'Bisq. [...]" was visible. Banks that do OCR processing could flag users' accounts. We should add a softbreak to the text, so that it's never near the QR Code.
bisq: PR Review bisq-network/bisq#7008 dev https://github.com/bisq-network/bisq/pull/7008

Concept ACK

Good catch! The toString() comparison is indeed bad.

I saw that you're using a AtomicLongs to count the item. Does this imply that detectMultipleHolderNames() is called from multiple threads? If that's the case suspiciousDisputesByTraderMap shouldn't be a HashMap.

Otherwise the following change is simpler and faster:

diff --git a/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java b/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java index 1be96332ef..09c722eed5 100644 --- a/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java +++ b/core/src/main/java/bisq/core/support/dispute/agent/MultipleHolderNameDetection.java @@ -146,7 +146,10 @@ public class MultipleHolderNameDetection {      ///////////////////////////////////////////////////////////////////////////////////////////        public void detectMultipleHolderNames() { -        String previous = suspiciousDisputesByTraderMap.toString(); +        int previous = suspiciousDisputesByTraderMap.values().stream() +                .mapToInt(List::size) +                .sum(); +          getAllDisputesByTraderMap().forEach((key, value) -> {              Set<String> userNames = value.stream()                      .map(dispute -> { @@ -161,8 +164,12 @@ public class MultipleHolderNameDetection {                  suspiciousDisputesByTraderMap.put(key, value);              }          }); -        String updated = suspiciousDisputesByTraderMap.toString(); -        if (!previous.equals(updated)) { + +        int updated = suspiciousDisputesByTraderMap.values().stream() +                .mapToInt(List::size) +                .sum(); + +        if (previous != updated) {              listeners.forEach(Listener::onSuspiciousDisputeDetected);          }      }
bisq: PR Review bisq-network/bisq#7012 dev https://github.com/bisq-network/bisq/pull/7012

Nit

getdaostatus is missing in the /bisq-cli --helpoutput.
bisq: PR Review bisq-network/bisq#7023 dev https://github.com/bisq-network/bisq/pull/7023

Nit

I know that someone else wrote the original code but we should try to clean up existing code when working on it. We can avoid the redudant copying of the trade list by using Collections.unmodifiableList(getObservableList()).

Apart from that I aggree with dutu. API consumers should be able to tell whether a trade failed.
bisq: Test Maker Transaction Vouts dev https://github.com/bisq-network/bisq/pull/7013
bisq: Refactor and Add Maker Transaction Vouts Tests dev https://github.com/bisq-network/bisq/pull/7014
bisq: Test All Maker BTC Cases dev https://github.com/bisq-network/bisq/pull/7015
bisq: Test All Taker BTC Cases dev https://github.com/bisq-network/bisq/pull/7019
  • Add takerInvalidFeeBtcAddressTest
  • Test all Taker BTC cases
    • takerCheckFeeAddressBtcInvalidFeeAddress
    • takerCheckFeeAddressBtcTooOldValidFee
    • takerExactFeeMatchBtcTest
    • takerHigherBtcFeeThanExpected
    • takerLowerButWithinToleranceBtcFee
    • takerPassFilterFeeCheck
    • takerFailFilterFeeCheck
    • takerNoFilterFeeMatchesDifferentDaoParameter
    • takerNoFilterFeeTooLow
bisq: Test all Maker BSQ Cases dev https://github.com/bisq-network/bisq/pull/7020
  • unconfirmedTransaction
  • newBsqTx
  • makerExactFeeMatchBsqTest
  • makerHigherBsqFeeThanExpected
  • makerLowerButWithinToleranceBsqFee
  • makerPassFilterFeeCheck
  • makerFailFilterFeeCheck
  • makerNoFilterFeeMatchesDifferentDaoParameter
  • makerNoFilterFeeTooLow
bisq: Test all Taker BSQ cases dev https://github.com/bisq-network/bisq/pull/7021
  • unconfirmedTransaction
  • newBsqTx
  • takerExactFeeMatchBsqTest
  • takerHigherBsqFeeThanExpected
  • takerLowerButWithinToleranceBsqFee
  • takerPassFilterFeeCheck
  • takerFailFilterFeeCheck
  • takerNoFilterFeeMatchesDifferentDaoParameter
  • takerNoFilterFeeTooLow
bisq: Implement AsyncFileWriter dev https://github.com/bisq-network/bisq/pull/7027
bisq: Implement AtomicFileWriter dev https://github.com/bisq-network/bisq/pull/7032

First, the AtomicFileWriter writes data to a rolling file and before swapping the rolling file with the active file. This makes all file writes atomic and let's Bisq recover from crashes.

Changes:
bisq2: Set Tor Log Level to 'notice' in Production dev https://github.com/bisq-network/bisq2/pull/1645
bisq2: Add second tor seednode dev https://github.com/bisq-network/bisq2/pull/1646
bisq2: Create Bisq Daemon dev https://github.com/bisq-network/bisq2/pull/1654
bisq2: Test TorSignatureUtils dev https://github.com/bisq-network/bisq2/pull/1665
  • signEmptyMessage
  • signMessage
  • verifyInvalidMessage
  • verifyInvalidSignature
bisq2: Fix Tor Address Validation Bugs dev https://github.com/bisq-network/bisq2/pull/1667
dev 15000 Total for items above.
alvasw commented 6 months ago

4d9274fac4abd43be4a4d9c6afb58e2b109f6ba4d0a8b4d6b6eff878bb4da427

ripcurlx commented 6 months ago

@HenrikJannsen: Could you please ACK this CR, as I wasn't involved? Thanks! In general I think it is sufficient for the future if the lead devs of each sub project ACK CRs. As I'm not very involved in Bisq right now I don't think my ACK is very helpful for anyone. WDYT?

HenrikJannsen commented 6 months ago

ACK

ghost commented 6 months ago

Issuance by Team:

team amount BSQ amount USD
dev 8720.93 15000.00

Total Issuance: 8720.93 BSQ (equivalent to: 15000.00 USD)

MwithM commented 6 months ago

Closed as accepted.