What is the reason this is a trait and not inherent methods? Do people generally need to abstract over merging different items?

I think it makes sense to have it as a trait, but I would envision the trait to be something more like Add.

trait Merge<With> {
    fn merge(&mut self, other: With) -> Result<(), Error>;
}

So that we might be able to define more structured Merging (e.g., merge raw tx into PSBT, PSBT V1 into V2 but not V2 into V1, etc).

However this is likely overkill v.s. as @Kixunil points out, an inherent method for it (which can also just be a wrapper around the Map type).

@JeremyRubin if someone actually has need for that (which I won't believe until I see an actual real-life example of code abstractly working on those types) then we can add With backward-compatibly by defaulting it to Self.

Seconding "let's make this an intrinsic method".

Also, I'd like to call it combine instead of merge.

Updated PR as suggested:

• Used 'combine' instead of 'merge', this makes the diff a bit bigger to change a bunch of identifiers that use 'merge'.
• Implemented methods directly on the types instead of a trait.

Also improved the unit tests

• Used files to hold the hex strings
• Added a comutative combine test

i'm kinda meh on changing the name from merge / combine since it's a breaking change for no great reason.

Can you add a deprecated wrapper around combine called merge? We can then remove it after a release or two...

The old name was confusing and inconsistent with PSBT terminology, which is how it got deleted originally without anybody raising their eyebrows.

But yes, we should put a deprecated wrapper around it.

I don't think we can deprecate it because the API break was making Map private. We can't use deprecate on that change. Unless I"m missing something?

If we really want to use deprecate we could make Map public again then add deprecate massage to each of the trait methods individually then make Map private after another release? Seems like a lot of hooh-hah considering the taproot release is so massively breaking anyways.

Ah, ok, sounds good.

If the merge method was actually just part of the Map trait, which made no sense from an API perspective anyway, I'm fine dropping it without ta deprecation.

@JeremyRubin it would be weird to add a new deprecated method that didn't exist before, even if it had the same name as a method on a deleted trait.

Yeah, users would still get failure on non-existing trait.

ah it used to exist as an inherent in 0.26 https://docs.rs/bitcoin/0.26.2/bitcoin/util/psbt/struct.PartiallySignedTransaction.html#method.merge, i guess in 0.27 it was moved to the trait, I wasn't sure when the refactor to Map was (altho it looks like it was a part of 0.27)

Lol, given that it existed in 0.26 I actually wouldn't mind re-adding and deprecating it now. But I would mildly prefer not to.

whatever works!