Skip to content
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

For Cycle 56 #1489

Closed
alvasw opened this issue Feb 23, 2024 · 5 comments
Closed

For Cycle 56 #1489

alvasw opened this issue Feb 23, 2024 · 5 comments
Assignees
Labels
parsed:valid https://bisq.wiki/Compensation#Ensure_your_request_is_valid team:dev https://bisq.wiki/Dev_Team was:accepted Indicates that a compensation request was accepted by DAO voting
Milestone

Comments

@alvasw
Copy link

alvasw commented Feb 23, 2024

Summary

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

  • BSQ requested: 8720.93
  • USD requested: 15000
  • BSQ rate: 1.72 USD per BSQ
  • Previous compensation request (if applicable): For Cycle 55 #1464

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 bisq-network/bisq#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 bisq-network/bisq#7007

ACK

bisq: PR Review bisq-network/bisq#7025 dev bisq-network/bisq#7025

ACK

bisq: PR Review bisq-network/bisq#7005 dev bisq-network/bisq#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 bisq-network/bisq#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 bisq-network/bisq#7012

Nit

getdaostatus is missing in the /bisq-cli --helpoutput.

bisq: PR Review bisq-network/bisq#7023 dev bisq-network/bisq#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 bisq-network/bisq#7013
bisq: Refactor and Add Maker Transaction Vouts Tests dev bisq-network/bisq#7014
bisq: Test All Maker BTC Cases dev bisq-network/bisq#7015
bisq: Test All Taker BTC Cases dev bisq-network/bisq#7019
  • Add takerInvalidFeeBtcAddressTest
  • Test all Taker BTC cases
    • takerCheckFeeAddressBtcInvalidFeeAddress
    • takerCheckFeeAddressBtcTooOldValidFee
    • takerExactFeeMatchBtcTest
    • takerHigherBtcFeeThanExpected
    • takerLowerButWithinToleranceBtcFee
    • takerPassFilterFeeCheck
    • takerFailFilterFeeCheck
    • takerNoFilterFeeMatchesDifferentDaoParameter
    • takerNoFilterFeeTooLow
bisq: Test all Maker BSQ Cases dev bisq-network/bisq#7020
  • unconfirmedTransaction
  • newBsqTx
  • makerExactFeeMatchBsqTest
  • makerHigherBsqFeeThanExpected
  • makerLowerButWithinToleranceBsqFee
  • makerPassFilterFeeCheck
  • makerFailFilterFeeCheck
  • makerNoFilterFeeMatchesDifferentDaoParameter
  • makerNoFilterFeeTooLow
bisq: Test all Taker BSQ cases dev bisq-network/bisq#7021
  • unconfirmedTransaction
  • newBsqTx
  • takerExactFeeMatchBsqTest
  • takerHigherBsqFeeThanExpected
  • takerLowerButWithinToleranceBsqFee
  • takerPassFilterFeeCheck
  • takerFailFilterFeeCheck
  • takerNoFilterFeeMatchesDifferentDaoParameter
  • takerNoFilterFeeTooLow
bisq: Implement AsyncFileWriter dev bisq-network/bisq#7027
bisq: Implement AtomicFileWriter dev bisq-network/bisq#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 bisq-network/bisq2#1645
bisq2: Add second tor seednode dev bisq-network/bisq2#1646
bisq2: Create Bisq Daemon dev bisq-network/bisq2#1654
bisq2: Test TorSignatureUtils dev bisq-network/bisq2#1665
  • signEmptyMessage
  • signMessage
  • verifyInvalidMessage
  • verifyInvalidSignature
bisq2: Fix Tor Address Validation Bugs dev bisq-network/bisq2#1667
dev 15000 Total for items above.
@ghost ghost added parsed:valid https://bisq.wiki/Compensation#Ensure_your_request_is_valid team:dev https://bisq.wiki/Dev_Team labels Feb 23, 2024
@MwithM MwithM added this to the Cycle 56 milestone Feb 24, 2024
@MwithM MwithM added this to In Review in Compensation Request Board Feb 24, 2024
@alvasw
Copy link
Author

alvasw commented Feb 25, 2024

4d9274fac4abd43be4a4d9c6afb58e2b109f6ba4d0a8b4d6b6eff878bb4da427

@ripcurlx
Copy link
Contributor

@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
Copy link

ACK

@MwithM MwithM moved this from In Review to Review Complete in Compensation Request Board Feb 29, 2024
@MwithM MwithM moved this from Review Complete to Proposal Submitted in Compensation Request Board Feb 29, 2024
@MwithM MwithM added the was:accepted Indicates that a compensation request was accepted by DAO voting label Mar 9, 2024
@ghost
Copy link

ghost commented Mar 9, 2024

Issuance by Team:

team amount BSQ amount USD
dev 8720.93 15000.00

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

@MwithM
Copy link
Contributor

MwithM commented Mar 9, 2024

Closed as accepted.

@MwithM MwithM closed this as completed Mar 9, 2024
Compensation Request Board automation moved this from Proposal Submitted to Closed Mar 9, 2024
@alvasw alvasw mentioned this issue Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parsed:valid https://bisq.wiki/Compensation#Ensure_your_request_is_valid team:dev https://bisq.wiki/Dev_Team was:accepted Indicates that a compensation request was accepted by DAO voting
Projects
Development

No branches or pull requests

4 participants