Skip to content

Implement savemempool method and test #148

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

GideonBature
Copy link
Contributor

@GideonBature GideonBature commented May 5, 2025

Going by the conversations with tcharding for PR #116 on implementing one method at a time for easier review: This is the second method implementation savemempool which is a specific type that returns a (json null). Once this is approved, I’ll proceed with the next one.

@GideonBature GideonBature force-pushed the savemempool branch 4 times, most recently from 3d386c0 to e012e7d Compare May 5, 2025 21:21
@tcharding
Copy link
Member

For methods that return Null I don't think we need to add a type. We can just use an integration test to prove the return is null. If Core later changes the test will catch it. Then we change the 'Returns' column to 'returns nothing'.

For reference see submitblock in v17 (client and types).

@tcharding
Copy link
Member

Once this is approved, I’ll proceed with the next one.

Once we have a few of these merged I'd say you'll have the idea and then you can just smash out a bunch at a time. Good to iron out the strategy first though with these simple PRs, saves work in the long run, well done.

@GideonBature
Copy link
Contributor Author

For methods that return Null I don't think we need to add a type. We can just use an integration test to prove the return is null. If Core later changes the test will catch it. Then we change the 'Returns' column to 'returns nothing'.

For reference see submitblock in v17 (client and types).

Aright, Noted...

@GideonBature
Copy link
Contributor Author

Once this is approved, I’ll proceed with the next one.

Once we have a few of these merged I'd say you'll have the idea and then you can just smash out a bunch at a time. Good to iron out the strategy first though with these simple PRs, saves work in the long run, well done.

True...

@GideonBature GideonBature force-pushed the savemempool branch 2 times, most recently from 7d2a55a to 1299e34 Compare May 6, 2025 07:53
@tcharding
Copy link
Member

Hey mate, when I merge PRs in this repo I use the Bitcoin Core merge script. And because of that if there are mentions (using @ character) they get included in the merge commit, then later github ends up spamming the mentioned dev everytime the merge commit shows up someplace (I don't know exactly where I just know I got spammed heaps in the past). So in this repo, and rust-bitcoin please don't put mentions in the PR description.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Super happy with this man, the new return in v23 is exactly why this work is useful. Nice to have found success on the second try (after pruneblock).

@GideonBature
Copy link
Contributor Author

Hey mate, when I merge PRs in this repo I use the Bitcoin Core merge script. And because of that if there are mentions (using @ character) they get included in the merge commit, then later github ends up spamming the mentioned dev everytime the merge commit shows up someplace (I don't know exactly where I just know I got spammed heaps in the past). So in this repo, and rust-bitcoin please don't put mentions in the PR description.

Alright. Sorry for that...

@tcharding
Copy link
Member

No stress!

@tcharding
Copy link
Member

Hey mate, if you think of it. When you force push can you post a quick message saying either what the force push is or what the status of the PR is. Otherwise I have to check out the branch, context switch into this PR, then work out if my last review suggestions were seen to etc - this all takes brain power, of which I only have a limited amount.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 565702f

@tcharding tcharding merged commit 97327e1 into rust-bitcoin:master May 8, 2025
28 checks passed
@GideonBature
Copy link
Contributor Author

Hey mate, if you think of it. When you force push can you post a quick message saying either what the force push is or what the status of the PR is. Otherwise I have to check out the branch, context switch into this PR, then work out if my last review suggestions were seen to etc - this all takes brain power, of which I only have a limited amount.

True, I will start doing that now...

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.

2 participants