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

Change #67

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Change #67

merged 4 commits into from
Jan 31, 2024

Conversation

Sosthene00
Copy link
Contributor

Handling change on the Receiver

@Sosthene00
Copy link
Contributor Author

The last commit is a suggestion, the scan_transaction and scan_transaction_with_labels distinction didn't feel quite right now that we should always scan for change labels. I propose that we only keep scan_transactions_with_label, but rename it without the with_labels, and instead add a bool. If caller pass it false, we just totally skip all labels including change.
The rationale for this would be to allow wallet that have other change strategies that don't imply silent payment to just scan plain no label outputs. This should also slightly improve performance. Anyway it's up to the caller to decide, even if we of course strongly advise for using sp change and turn this flag on.
Alternative to that would be to not even add this bool and just say "ok we always scan for labels, and every wallet as at least a change label"

Copy link
Owner

@cygnet3 cygnet3 left a comment

Choose a reason for hiding this comment

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

Some minor stuff, looking good!

src/receiving.rs Show resolved Hide resolved
src/receiving.rs Outdated Show resolved Hide resolved
@Sosthene00 Sosthene00 force-pushed the change branch 2 times, most recently from 0f387a8 to bdcb483 Compare January 25, 2024 12:09
@cygnet3 cygnet3 merged commit 5389c72 into cygnet3:master Jan 31, 2024
2 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.

2 participants