Skip to content
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

Simplify DrainWeights/Target API #1

Closed
evanlinjin opened this issue Nov 29, 2023 · 2 comments · Fixed by #21
Closed

Simplify DrainWeights/Target API #1

evanlinjin opened this issue Nov 29, 2023 · 2 comments · Fixed by #21
Labels
enhancement New feature or request

Comments

@evanlinjin
Copy link
Member

Original discussion: bitcoindevkit/bdk#1072 (comment)

@jp1ac4 @darosior I'm working on some breaking changes to the API.

After a discussion with @LLFourn, we've decided to include DrainWeights in Target and keep Target in the CoinSelector. This simplifies the API and gives us an easy pathway to take into account the output count varint weight changes when including the drain output.

How this simplifies the API:

  • The target & drain weights are included in the CoinSelector, so they don't need to be passed in as method inputs.
  • Change policy can be simplified to just: Fn(&CoinSelector) -> bool.

How we can take into account the output varint weight changes when including drain:

pub struct Target {
    /*other fields*/

    pub base_weight: u32,
    pub drain_weights: DrainWeights,
}

impl Target {
    pub fn new(
        feerate: FeeRate,
        recipient_outputs: impl IntoIterator<Item = (u32 /*weight*/, u64 /*value*/)>
        drain_outputs: impl IntoIterator<Item = DrainWeights>,
    ) -> Self {
        todo!()
    }
}

This calculates the base_weight and the drain_weight (which includes output varint changes) that are both included in Target.

@evanlinjin evanlinjin added the enhancement New feature or request label Nov 29, 2023
@evanlinjin
Copy link
Member Author

@LLFourn's thoughts (bitcoindevkit/bdk#1072 (comment)):

So in my mind Target and DrainWeights would still be separate things just both exposed and settable publicly. Target would include the number of outputs and DrainWeights would include the number of outputs in the draining output. I also think you need to include the "base weight" in Target as it is coupled to the number of outputs.

Now I thought further -- if we create this invariant I think this also allows us to reduce the change_policy to a single integer -- i.e. the threshold of excess coin which should trigger the inclusion of the drain output. Wdyt @evanlinjin?

@LLFourn
Copy link
Collaborator

LLFourn commented Dec 22, 2023

Note I did the change policy as a single value in #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants