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

Select Spendable Streamlined #1143

Merged
merged 50 commits into from
May 28, 2024

Conversation

fluidvanadium
Copy link
Contributor

@fluidvanadium fluidvanadium commented May 27, 2024

This is a rewrite of select_spendable_notes, using what we have learned over the past couple of weeks.

Notable improvement is that the ignore_dust test goes from passing to failing for zip317

Copy link
Member

@zancas zancas left a comment

Choose a reason for hiding this comment

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

I left some notes.

@fluidvanadium fluidvanadium force-pushed the select_spendable-X0 branch 2 times, most recently from d3b93a5 to 0a083a0 Compare May 28, 2024 04:52
@fluidvanadium fluidvanadium requested a review from zancas May 28, 2024 04:54
Copy link
Contributor

@Oscar-Pepper Oscar-Pepper left a comment

Choose a reason for hiding this comment

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

I have left some comments which i think are worth implementing but non-blocking. The reason why I am requesting changes is because our expected fees seem to just be inputted by the test writer with no real reviewed expected fee calculator and some of the expected fees seem like they might be wrong to me. ALSO the new select_spendable_notes has lost control of selecting notes based on pool which i think is more important than value as it effects our pool policies and also is needed to reduce fees for user. although I havent worked out how this is even possible with the current LRZ code as the use_orchard / use_sapling booleans dont seem to work correctly or get fed into this function, so non-blocking... maybe we should implement this soon though.

Copy link
Contributor

@Oscar-Pepper Oscar-Pepper left a comment

Choose a reason for hiding this comment

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

wonderful

@fluidvanadium fluidvanadium merged commit 83e8c33 into zingolabs:dev May 28, 2024
17 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.

3 participants