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

Revert setting of null to deposit tx id #3958

Merged
merged 3 commits into from
Feb 10, 2020

Conversation

ripcurlx
Copy link
Contributor

As we saw some unexpected behavior during testing we decided to move this change to v1.2.7

Additionally we'll remove even more information from the trade statistics objects and will deploy it together with changes
in the offer objects.

@ripcurlx ripcurlx requested a review from sqrrm February 10, 2020 12:29
public boolean equals(Object obj) {
if (this == obj) return true;
if (obj == null || getClass() != obj.getClass()) return false;
var wrapper = (WrapperTradeStatistics2) obj;
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need a check that obj is actually a WrapperTradeStatistics2 here? Could cause an exception otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could add it to make it safer if it is reused somewhere.
Atm using it like that should not cause this case to ever happen.

.map(WrapperTradeStatistics2::new)
.distinct()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But will add a check to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah wait - I've added it already 😉

 if (obj == null || getClass() != obj.getClass()) return false;

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, that's right, missed it.

Co-Authored-By: sqrrm <[email protected]>
@sqrrm
Copy link
Member

sqrrm commented Feb 10, 2020

Perhaps we should not leave this one as @Nullable in TradeStatistics2

    @Nullable
    private final String depositTxId;

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

ACK

It's fine that depositTxId stays @Nullable since it's not set to null.

Tested with Alice and Bob on version 1.2.6, 1.2.5 respectively and Carol as a neutral spectator on both 1.2.5 and 1.2.6.

@sqrrm sqrrm merged commit 923487e into bisq-network:master Feb 10, 2020
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Feb 17, 2020
@ripcurlx ripcurlx deleted the revert-setting-deposittxid-null branch July 16, 2021 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants