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

Refactor replacement outputs to be an Iterator of ref TxOuts #529

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benalleng
Copy link
Contributor

We can pass the replacement outputs as a ref Iterator instead of using a Vec of TxOuts.

Same as before I guess the question is whether this is really needed especially considering how little was affected downstream by this arg type change.

Perhaps its still useful to convert this to an Iterator and not a reference but I'm not totally sure, especially since in my approach I end up just converting it back to a Vec to work with interleave_shuffle better

We can pass the replacement outputs as a ref Iterator instead of
using a Vec of TxOuts.
@benalleng benalleng changed the title Refactor replacement outputs to be a ref Iterator of TxOuts Refactor replacement outputs to be an Iterator of ref TxOuts Feb 5, 2025
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 13166226056

Details

  • 8 of 14 (57.14%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.08%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/receive/v2/mod.rs 0 6 0.0%
Totals Coverage Status
Change from base Build 13161838713: 0.0%
Covered Lines: 3765
Relevant Lines: 4761

💛 - Coveralls

@spacebear21
Copy link
Collaborator

Similar to #527 I think using references here adds complexity for unclear benefits.

I'd be fine with changing it to replacement_outputs: impl IntoIterator<Item = TxOut>, if interleave_shuffle can be made to work with the same type. But if we're converting back into a Vec anyway it's probably not worth it.

@DanGould
Copy link
Contributor

DanGould commented Feb 7, 2025

The reason to do it here is different than just changing ownership to a ref. Perhaps the iterator is a db struct and not an in-memory Vec, which is where a more generic interface is helpful. I did some LLMing to get interleave_shuffle to also take an interator which is I think what needs to happen to realize the generalization here

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.

4 participants