-
Notifications
You must be signed in to change notification settings - Fork 111
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
Reply to Request::MempoolTransactionIds
with mempool content
#2720
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already looks good! I added a two small suggestions, but they're optional 👍
Thanks @jvff , this was supposed to be a draft PR but i submitted it late yesterday and didn't realized it ended up as a normal one. Anyway, will do your suggestions asap. Thanks again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🎉
Motivation
Now that we have a mempool we can start responding with it contents to some inbound messages.
Close #1083 when merged.
Solution
This is the easier message that responds with the ids available in the mempool. Most of the work is in the test.
Review
I used some code from other tests made by @jvff so i think it will be a good candidate to the an initial review. On the other hand as we are all in mempool pretty much anyone can review so feel free to send feedback.
Reviewer Checklist