Skip to content

Implement encryptwallet method and test #250

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 2 commits into from
Jun 19, 2025

Conversation

GideonBature
Copy link
Contributor

The JSON-RPC method encryptwallet does return a type. We want to test this to catch any changes in behavior in future Core versions.

This PR adds a client function that errors if the return value is anything other than the type it returns, along with an integration test that calls this function.

Ref: #116

@GideonBature GideonBature marked this pull request as draft June 16, 2025 06:44
@GideonBature GideonBature marked this pull request as ready for review June 16, 2025 07:50
Copy link
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK e9bf939

@jamillambert
Copy link
Collaborator

Nice, you're on a roll.

@GideonBature
Copy link
Contributor Author

Nice, you're on a roll.

Thank you!

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.

Everything looks good. If you don't want to do the auth thing just throw a comment in the code, something like // TODO: This match shoud really be handled by the crate not in each individual test like this.

Cheers

Comment on lines 164 to 171
let client = match () {
#[cfg(feature = "v20_and_below")]
() => node.create_wallet(wallet_name).expect("createwallet"),
#[cfg(not(feature = "v20_and_below"))]
() => {
node.client.create_wallet(wallet_name).expect("createwallet");
&node.client
},
Copy link
Member

Choose a reason for hiding this comment

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

Are you feeling adventurous? It would be nice to investigate why this is needed and move the feature gating logic to node/src/lib.rs in the create_wallet function so that every test author does not have to think about it. FTR I did not investigate, I vaguely remember hitting this before though so my suggestion is just a hunch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I am even thinking that we can just use the create_wallet function from node, since it returns a client, then we can just call the encrypt_wallet on that client directly, I think it should work with all the versions, so we need not feature gate the test any more.

The create_wallet from node works for v17-v20 that needs the full wallet uri and for v21 upwards which also accepts either just the wallet name or the full wallet uri.

@GideonBature GideonBature force-pushed the encryptwallet branch 4 times, most recently from 078e1f7 to 7e50e41 Compare June 18, 2025 10:39
Copy link
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK e513cbe

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 e513cbe

@tcharding tcharding merged commit b518425 into rust-bitcoin:master Jun 19, 2025
29 checks passed
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.

3 participants