Skip to content

Address all TODO in v22 #295

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 4 commits into from
Jul 19, 2025
Merged

Conversation

jamillambert
Copy link
Collaborator

@jamillambert jamillambert commented Jul 15, 2025

Go through all the TODO in the v22 types table. Add all the missing RPCs.

Redefine listdescriptors in v25 due to added return field.

enumeratesigners and walletdisplayaddress RPCs require an external signer and have been left untested because of this. Opened #294.

The heading comment for raw_transactions was missing from verify v26.

Add it.
Add the struct to types.
Add the client macro.
Update the table to version, no model is required. Currently untested.
Add the reexports.
Add the struct, client macro, test and reexports.

Redefine the struct in v25 for added return field.

Change to no_model since there is nothing to model.
pub internal: Option<bool>,
/// Defined only for ranged descriptors.
pub range: Option<[u64; 2]>,
/// The next index to generate addresses from; defined only for ranged descriptors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// The next index to generate addresses from; defined only for ranged descriptors.
/// Same as `next_index` field. Kept for compatibility reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Address(ref e) => Some(e),
}
}
}
Copy link
Member

@tcharding tcharding Jul 15, 2025

Choose a reason for hiding this comment

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

We don't need all this error stuff man, we can just return the address::ParseError directly since there is only one error. I have a vague feeling you added a single field enum recently in some other PR and I missed observing this, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed the error enum. I'll have a look for the other one.

Copy link
Collaborator Author

@jamillambert jamillambert Jul 16, 2025

Choose a reason for hiding this comment

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

Found the other one and removed it in #300

Add the struct, model, client macro, and reexports.

Currently untested.
@jamillambert
Copy link
Collaborator Author

Suggested changes made

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 805c757

@tcharding
Copy link
Member

Man I was super confused because there is a docs change in the final patch instead of the one before - I thought I had a jj fetching problem. Merging anyways - YOLO.

@tcharding tcharding merged commit 24607b7 into rust-bitcoin:master Jul 19, 2025
30 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.

2 participants