Skip to content

Conversation

@0xZaddyy
Copy link
Contributor

@0xZaddyy 0xZaddyy commented Dec 11, 2025

Summary

This PR Audits serialization trait implementations across the codebase to prevent unintentional API exposure and reduce dependencies.

Closes #1147

Pull Request Checklist

Please confirm the following before requesting review:

@coveralls
Copy link
Collaborator

coveralls commented Dec 11, 2025

Pull Request Test Coverage Report for Build 20601614417

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.902%

Totals Coverage Status
Change from base Build 20575392805: 0.0%
Covered Lines: 9668
Relevant Lines: 11662

💛 - Coveralls

@nothingmuch
Copy link
Collaborator

nothingmuch commented Dec 12, 2025

I don't know about the serde usage in the ffi stuff and how appropriate it is to remove it for those types

Only commenting about the removal and feature gating in the payjoin crate: i don't think a feature flag for this is desirable because:

  • it's one more feature flag, or 2x the feature flag combinations, which are already intractable for us to test
  • serialization is required as part of persistence for correct implementation of the protocol, and not all struct fields are pub

so with the feature disabled the crate couldn't be used to write a safe implementation

so i think this dependence on serde in our public api is actually desirable and intended

@nothingmuch nothingmuch requested review from spacebear21 and removed request for arminsabouri December 12, 2025 01:11
@spacebear21
Copy link
Collaborator

Removing the De/Serialize from payjoin-ffi seems appropriate, they don't do anything there and were probably just copied over unintentionally. Agreed about keeping serde tightly coupled with rust-payjoin for persistence.

 Remove unintended serde derives from types that shouldn't be serializable by default. Conditionally enable serialization behind feature flag to prevent accidental API exposure through serialization.
@0xZaddyy 0xZaddyy force-pushed the audit-trait-implementations branch 4 times, most recently from ad78270 to a7352f8 Compare December 30, 2025 16:57
@0xZaddyy 0xZaddyy force-pushed the audit-trait-implementations branch from a7352f8 to 9c33ef1 Compare December 30, 2025 16:58
@0xZaddyy 0xZaddyy marked this pull request as ready for review December 30, 2025 17:27
@spacebear21 spacebear21 merged commit 013615b into payjoin:master Dec 31, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

audit types for unintentional trait implementations

4 participants