[xstoicunicornx]

Rename SenderBuilderError to BuildSenderError and
ReceiverBuilderError to BuildReceiverError in payjoin-ffi so the FFI error names mirror the core payjoin crate's naming convention (e.g. payjoin::send::BuildSenderError).

This is a follow up from suggestion made in #1724 (comment).

Claude Fable was used to make these code updates.

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Coverage Report for CI Build 29438188507

Coverage remained the same at 86.187%

Details

• Coverage remained the same as the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	15963
Covered Lines:	13758
Line Coverage:	86.19%
Coverage Strength:	345.24 hits per line

---

💛 - Coveralls

[benalleng]

Maybe you could make this Builder to prevent the collision?

[xstoicunicornx]

This was very helpful thanks! How did you know this was what was causing the collision? I didn't really know where to start troubleshooting.

[benalleng]

Just experience dealing with the dart binding generator. We dealt with the same collision in #1724 (comment) and knowing how dart uniffi smooshes names together.

I suspected that SenderError::Build() became BuildSenderException which collides with the struct BuildSenderError which also translates to BuildSenderException I double-checked by building the dart bindings on your branch and grepping to confirm in payjoin-ffi/dart/lib/payjoin.dart. This is especially confusing as the enum SenderError::Build(Arc<BuildSenderError>) wraps the offending Struct in an Arc.

Now it does not collide as we have BuildSenderException and BuildSenderSenderException. Can't say I love that but thats just how dart is

[caarloshenriq]

Concept ACK 00a3f62

One thing to double check: SenderInputError also has a Build(Arc) variant (send/error.rs, untouched by this commit) that looks like the exact same pattern @benalleng flagged for SenderError::Build, variant name Build wrapping a struct that also resolves to BuildSenderException in dart. Was SenderInputError checked the same way (building dart bindings and grepping payjoin.dart), or did that one slip through since the earlier check was specific to SenderError?

[xstoicunicornx]

One thing to double check: SenderInputError also has a Build(Arc) variant (send/error.rs, untouched by this commit) that looks like the exact same pattern @benalleng flagged for SenderError::Build, variant name Build wrapping a struct that also resolves to BuildSenderException in dart. Was SenderInputError checked the same way (building dart bindings and grepping payjoin.dart), or did that one slip through since the earlier check was specific to SenderError?

Can you be a bit more specific on what should be double checked here? What Ben was referring to above was a duplicate compiled variable name that caused a collision specifically in Dart bindings. This was the enum that had compiled variable name that collided with my error renaming.

rust-payjoin/payjoin-ffi/src/send/error.rs

Line 196 in bd27d42

Build(Arc<SenderBuilderError>),

@DanGould I can't remember whether you said that FFI 1.0 had gone out already or not. If not this might be good to get in, if it has then we should probably close this.

[benalleng]

So the payjoin 1.0 FFI releases have been pushed but I'm not sure if that means that this should be closed by default.

Do FFI bindings have the same semver expectations as the main rust code? Especially considering none of the bindings versions are post 1.0 I would lean towards no and that this can still be merged.

[benalleng]

ACK 00a3f62

Thinking more about my previous comment I think that the breaking change expectation is not as strict in the FFI layer as it is in the rust layer and that this is still appropriate to merge post payjoin-1.0. All the bindings are still relatively immature and their is a possibility that things like the new uniffi-dart versions or some existing language specific bug will need fixing and this should not limit us across the entire repo.

@DanGould looking for an ACK on this above sentiment to merge this PR

[DanGould]

payjoin-ffi is still pre 1.0 and breaking changes are welcome there. Payjoin-1.0 lets ffi commit to the event format underneath while ffi still moves. And I think with uniffi being pre 1.0 and much less stable than say, bitcoin-0.32, constantly changing minor versions, payjoin-ffi-1.0 is probably not on the near horizon.