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

Redact DepositTxId when publishing TradeStatistics for user privacy #3891

Conversation

wiz
Copy link
Contributor

@wiz wiz commented Jan 11, 2020

Fixes #3893 by redacting the Bitcoin TXID from TradeStatistics2 objects.

Before:

  { 
    "currency": "JPY",
    "direction": "SELL",
    "tradePrice": 8791986900,
    "tradeAmount": 10000,
    "tradeDate": 1578784489588,
    "paymentMethod": "F2F",
    "offerDate": 1578784398352,
    "useMarketBasedPrice": true,
    "marketPriceMargin": 0.0,
    "offerAmount": 10000,
    "offerMinAmount": 10000,
    "offerId": "12635-224f7143-3366-46e7-9e14-7fa6f39fcb2b-125",
    "depositTxId": "9c67453e57cfc80e2c121caf54f8f739cef6c5d7e9afdceec7843436a920f9d8",
    "currencyPair": "BTC/JPY",
    "primaryMarketDirection": "SELL",
    "primaryMarketTradePrice": 87919869000000,
    "primaryMarketTradeAmount": 10000,
    "primaryMarketTradeVolume": 8791980000
  },

After:

  {
    "currency": "JPY", 
    "direction": "SELL",
    "tradePrice": 8791986900,
    "tradeAmount": 10000, 
    "tradeDate": 1578784489149,
    "paymentMethod": "F2F", 
    "offerDate": 1578784398352,
    "useMarketBasedPrice": true,
    "marketPriceMargin": 0.0,
    "offerAmount": 10000, 
    "offerMinAmount": 10000, 
    "offerId": "12635-224f7143-3366-46e7-9e14-7fa6f39fcb2b-125",
    "depositTxId": "0000000000000000000000000000000000000000000000000000000000000000",
    "currencyPair": "BTC/JPY",
    "primaryMarketDirection": "SELL",
    "primaryMarketTradePrice": 87919869000000,
    "primaryMarketTradeAmount": 10000, 
    "primaryMarketTradeVolume": 8791980000
  },

@wiz wiz requested a review from ripcurlx as a code owner January 11, 2020 23:45
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK

This would cause duplicated trade statistics if one trader has old verison and one trader has new version as the tx id is used for the hash. We need to fade that in with an activation date so the risk that old users are part of the trade is much lower. Also it should be declared nullable, set to null instead of the "0" string and handled accordingly in the protobuf methods and a comment should be added about the reasoning.

@wiz
Copy link
Contributor Author

wiz commented Jan 16, 2020

@chimp1984 ah yes, those are some very good points. would you prefer to make the fix? it sounds like you want to do it 😅

@chimp1984
Copy link
Contributor

I will not have time and will step out of Bisq after that cycle.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ripcurlx
Copy link
Contributor

Closing as superseded by #3911.

@ripcurlx ripcurlx closed this Jan 21, 2020
@wiz wiz deleted the redact-deposit-txid-in-tradestatistics-for-privacy branch January 21, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bisq nodes leak TXID of every trade when TradeStatistics are generated
3 participants