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

Coin Selection doesn't consider the minimum ADA for change #553

Open
abdelkrimdev opened this issue Nov 24, 2022 · 8 comments
Open

Coin Selection doesn't consider the minimum ADA for change #553

abdelkrimdev opened this issue Nov 24, 2022 · 8 comments
Assignees

Comments

@abdelkrimdev
Copy link

add_inputs_from [LargestFirstMultiasset] doesn't consider the minimum ADA for change output, thus sometimes we don't have enough ADA in a tx's inputs to cover for minimum ADA in change output if we call add_inputs_from then call add_change_if_needed we get Not enough ADA leftover to include non-ADA assets in a change address

@lisicky
Copy link
Contributor

lisicky commented Nov 24, 2022

Hi @abdelkrimdev! It's a known issue, the current implementation of add_inputs_from doesn't consider a change value. Before we fix it, I suggest adding one more input with only ada for a temporary workaround. Or you can add one output with an expected change and call the add_inputs_from and after that, delete the output.

@abdelkrimdev
Copy link
Author

Yes, I already did that, but it's a hack, and it leads to some potential bugs in the future. I even built a mini coin selection here:
https://github.com/MartifyLabs/mesh/blob/main/packages/module/src/core/CIP2.ts
but fixing this at the CSL level could be much better.

@AdamMachera
Copy link

@lisicky when are you guys planning to fix this issue?

@Emurgo Emurgo deleted a comment Feb 20, 2023
@lisicky
Copy link
Contributor

lisicky commented Feb 20, 2023

@AdamMachera fix requires small redesign inside tx builder. I would say fix will be in 11.5 release. I will try to make it faster

@tqueri
Copy link

tqueri commented Apr 19, 2023

@AdamMachera fix requires small redesign inside tx builder. I would say fix will be in 11.5 release. I will try to make it faster

Thanks @lisicky if I can help with anything let me know, I have a limited skillset in rust but for testing or anything

@bugii
Copy link

bugii commented Jul 17, 2023

Hi @abdelkrimdev! It's a known issue, the current implementation of add_inputs_from doesn't consider a change value. Before we fix it, I suggest adding one more input with only ada for a temporary workaround. Or you can add one output with an expected change and call the add_inputs_from and after that, delete the output.

@lisicky Regarding this workaround: What is the recommended way to remove an output from a TransactionBuilder?

@AdamMachera
Copy link

@tqueri has it been fixed in version 11.5?

@BEST-Thanawat
Copy link

BEST-Thanawat commented Sep 14, 2023

this error "Not enough ADA leftover to include non-ADA assets in a change address" happened to me when I used with_asset_and_min_required_coin which is DEPRECATED so I used with_asset_and_min_required_coin_by_utxo_cost instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants