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

Fix small p2p network issues #3154

Merged
merged 8 commits into from
Aug 29, 2019

Conversation

chimp1984
Copy link
Contributor

No description provided.

@ripcurlx ripcurlx changed the title Fix small p2 p network issues Fix small p2p network issues Aug 28, 2019
@ripcurlx ripcurlx changed the title Fix small p2p network issues [WIP] Fix small p2p network issues Aug 28, 2019
@ripcurlx
Copy link
Contributor

@chimp1984 I've added [WIP] to your title, so it is easier to see that you are still working on this.

@chimp1984 chimp1984 changed the title [WIP] Fix small p2p network issues Fix small p2p network issues Aug 28, 2019
@chimp1984 chimp1984 marked this pull request as ready for review August 28, 2019 19:07
@chimp1984 chimp1984 requested a review from ripcurlx as a code owner August 28, 2019 19:07
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.

utACK - Looks good to me. Thanks for the PR! 👍

@ripcurlx
Copy link
Contributor

Maybe @sqrrm or @ManfredKarrer could give it a quick ACK again. From my side changes seem to be fine.

@christophsturm
Copy link
Contributor

looks good. some of the commits messages could be improved by explaining why the change was done, and also the pr description could explain the overall motivation.

@ripcurlx
Copy link
Contributor

looks good. some of the commits messages could be improved by explaining why the change was done, and also the pr description could explain the overall motivation.

@chimp1984 Yes, please just add a small description for your PRs in the future. Everything looks perfectly fine, what you are contributing, but it helps to review if there is more context.

Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

ACK.

though, next time, please leave the formatting as is. it is very hard to spot the differences that actually change something.

@freimair freimair merged commit 4e7cb04 into bisq-network:master Aug 29, 2019
@christophsturm
Copy link
Contributor

though, next time, please leave the formatting as is. it is very hard to spot the differences that actually change something.

also a good idea: reformat in a separate commit

@chimp1984
Copy link
Contributor Author

the commits messages could be improved by explaining why the change was done, and also the pr description could explain the overall motivation.

We do better next time.

please leave the formatting as is.

I use the formatting settings from the IDEA which are part of the project and as far I understand formatting should be applied before commit.

reformat in a separate commit

Agree.

@chimp1984 chimp1984 deleted the Fix-small-P2P-network-issues branch August 29, 2019 19:35
/**
* Marker interface for messages which must not be added again after a remove message has been received (e.g. MailboxMessages).
*/
public interface AddOncePayload {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue number that this change fixes? After reviewing and refactoring this code I'm having trouble understanding the bug that would have needed this fix. I'd like to try and reproduce it in my tests, if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants