Skip to content

Conversation

@Mshehu5
Copy link
Contributor

@Mshehu5 Mshehu5 commented Jan 6, 2026

This is a followup to #1254 to address #1254 (comment)

I fear this test is a silent error. Shouldn't contributing duplicate inputs cause error rather than continue and produce the same result with differing input?

Previously, contribute_inputs() filtered out duplicate inputs when the same outpoint was provided multiple times.
This PR makes it cause an error

Pull Request Checklist

Please confirm the following before requesting review:

@coveralls
Copy link
Collaborator

coveralls commented Jan 6, 2026

Pull Request Test Coverage Report for Build 20795200310

Details

  • 14 of 17 (82.35%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 82.908%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/core/receive/error.rs 0 3 0.0%
Totals Coverage Status
Change from base Build 20717534858: -0.001%
Covered Lines: 9682
Relevant Lines: 11678

💛 - Coveralls

@DanGould
Copy link
Contributor

DanGould commented Jan 7, 2026

May I understand your rationale between the two? My question was genuine. It wasn't an instruction in disguise.

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable approach. Please consider the comment formatting to regular complete sentences.

@Mshehu5
Copy link
Contributor Author

Mshehu5 commented Jan 7, 2026

May I understand your rationale between the two? My question was genuine. It wasn't an instruction in disguise.

While working on #1254 based on the discussion by other contributors, it was noted in the issue that:

Maybe it should filter for unique new inputs.

After implementing the filtering I also considered that erroring out might be a better approach. However I didn’t add this because it wasn’t mentioned in the issue and I was unclear whether erroring on duplicate inputs was the intended flow for contribute_inputs.

Based on payjoin-cli this function is mostly used only once so I assumed duplicate inputs might be handled gracefully (by filtering) rather than causing an error. Additionallyz if the inputs are insufficient or intentionally filled with duplicates the logic would still result in a ValueTooLow error if the total amount does not meet the required threshold.

These were some of the considerations behind my approach. The reason I submitted a minimal filtering PR was that if erroring out is the preferred behavior it would likely be flagged by other contributors (as you’ve done) which is why I proceeded with a follow-up PR.

Incase of next time I think is best I ask more questions in the issue or add notes for reviewers to consider in the PR to better address the issue

Previously, contribute_inputs() filtered out duplicate inputs
when the same outpoint was provided multiple times.

This commit changes the behavior to return an explicit error when
duplicate inputs are detected.
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.

3 participants