Skip to content

Conversation

@storopoli
Copy link
Contributor

A bunch of issues when porting a custom types.rs module from bitcoind-async-client to corepc-types.

  • v24::GetRawMempoolVerbose should use the same version MempoolEntry and not the v17.
  • v28::SubmitPackageTxResult might not have effective-includes field.
  • v24::ListUnspentItem might not have a label field.

/// Map of txid to [`MempoolEntry`].
#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
#[cfg_attr(feature = "serde-deny-unknown-fields", serde(deny_unknown_fields))]
pub struct GetRawMempoolVerbose(pub BTreeMap<String, MempoolEntry>);
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks man. Can you add an into_model method too please. Copy from v17 also, or here it is.

impl GetRawMempoolVerbose {
    /// Converts version specific type to a version nonspecific, more strongly typed type.
    pub fn into_model(self) -> Result<model::GetRawMempoolVerbose, MapMempoolEntryError> {
        use MapMempoolEntryError as E;

        let mut map = BTreeMap::new();
        for (k, v) in self.0.into_iter() {
            let txid = k.parse::<Txid>().map_err(E::Txid)?;
            let relative = v.into_model().map_err(E::MempoolEntry)?;
            map.insert(txid, relative);
        }
        Ok(model::GetRawMempoolVerbose(map))
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a5f9033

pub address: String,
/// The associated label, or "" for the default label.
pub label: String,
pub label: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

The need for this change proves what we suspect, our test coverage is not good enough (see #58).

cc @jamillambert

/// whose fees and vsizes are included in effective-feerate.
#[serde(rename = "effective-includes")]
pub effective_includes: Vec<String>,
pub effective_includes: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

And this one too. Thanks for finding and fixing these for us @storopoli.

tcharding
tcharding previously approved these changes Oct 2, 2025
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 d8e2ac1

@tcharding
Copy link
Member

Woops I acked forgetting that we need the into_model still.

@storopoli
Copy link
Contributor Author

Woops I acked forgetting that we need the into_model still.

Try now :)

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 5bb3b31

All looks good to me, thanks for the fix.

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 5bb3b31

@tcharding tcharding merged commit 827aed3 into rust-bitcoin:master Oct 3, 2025
30 checks passed
@tcharding
Copy link
Member

FTR it came to me this morning that during review I did not check if theses modified types had any newer counterparts in more recent Coer versions. Checked just now before merging.

@tcharding
Copy link
Member

Thanks for this @storopoli, super useful.

@storopoli
Copy link
Contributor Author

No worries. Can we cut a new release of corepc-types?

@tcharding
Copy link
Member

Yeah mate, sure thing. I'll do it on Monday for you.

@storopoli storopoli deleted the types/v29-fixes branch October 6, 2025 15:41
@storopoli
Copy link
Contributor Author

Don't forget the tag :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants