Skip to content

Avoid concrete rust-bitcoin type in version-specific GetOrphanTxs struct#490

Open
0xB10C wants to merge 1 commit intorust-bitcoin:masterfrom
0xB10C:2026-02-fix-484
Open

Avoid concrete rust-bitcoin type in version-specific GetOrphanTxs struct#490
0xB10C wants to merge 1 commit intorust-bitcoin:masterfrom
0xB10C:2026-02-fix-484

Conversation

@0xB10C
Copy link
Contributor

@0xB10C 0xB10C commented Feb 3, 2026

A concrete rust-bitcoin type was added to the version-specific GetOrphanTxs struct during #435. This is corrected by using a list of strings and converting to the model type via into_model.

Affects v29 and v30.

Fixes #484

jamillambert
jamillambert previously approved these changes Feb 3, 2026
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 1733130

@tcharding
Copy link
Member

Mad, thanks man. Can we use our funky explicit error style in the integration test to check all the re-exports are good?

In integration_test/tests/hidden.rs in hidden__get_orphan_txs__modelled something like:

    let model_v0: Result<mtype::GetOrphanTxs, GetOrphanTxsError> = json_v0.into_model();
    let model_v0 = model_v0.unwrap();

/// Error when converting a `GetOrphanTxs` type into the model type.
#[derive(Debug)]
pub enum GetOrphanTxsError {
/// Conversion of the transaction `txid` field failed.
Copy link
Member

@tcharding tcharding Feb 4, 2026

Choose a reason for hiding this comment

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

Since this error is used in multiple places and there isn't always a txid field perhaps something like this?

/// Conversion of a `txid` from the orphanage failed.

Copy link
Member

Choose a reason for hiding this comment

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

If that isn't too cute for your likeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used that. I lazily just copy and pasted from GetOrphanTxsVerboseOneEntryError before.

@tcharding
Copy link
Member

Thanks for coming back and picking this up mate, appreciate the work.

A concrete rust-bitcoin type was added to the version-specific
`GetOrphanTxs` struct during rust-bitcoin#435. This is corrected by using a list
of strings and converting to the model type via `into_model`.

Affects v29 and v30.

Also, use the explicit error in the itegration tests to check that
the re-exports are good.

Fixes rust-bitcoin#484
@0xB10C
Copy link
Contributor Author

0xB10C commented Feb 6, 2026

Addressed #490 (comment)!

// use explicit errors here to check that the re-exports are good
let model_v0: Result<mtype::GetOrphanTxs, GetOrphanTxsError> = json_v0.into_model();
let model_v1: Result<mtype::GetOrphanTxsVerboseOne, GetOrphanTxsVerboseOneEntryError> = json_v1.into_model();
let model_v2: Result<mtype::GetOrphanTxsVerboseTwo, GetOrphanTxsVerboseTwoEntryError> = json_v2.into_model();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does the formatting job also check the integration test? At least locally that should have been split over multiple lines

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.

Concrete rust-bitcoin type in hidden modules

3 participants