-
Notifications
You must be signed in to change notification settings - Fork 25
Improve autobalancing errors when change has no lovelace #816
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- cardano-api/src/Cardano/Api/Internal/Fees.hs: Language not supported
- cardano-api/test/cardano-api-test/Test/Cardano/Api/Orphans.hs: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
| L.isZero outValue -> pure () -- empty TxOut - ok, it's removed at the end | ||
| L.isZero coin -> -- no ADA, just non-ADA assets | ||
Left $ | ||
TxBodyErrorAdaBalanceTooSmall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to give @mkoura a heads up if he is depending on the error output for this failure case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have limited understanding of this code. But I am pretty confident it is all refactoring except for the addition of the TxBodyErrorAdaBalanceTooSmall
error, which I don't understand why it is there, but I imagine it is correct.
I just added a suggestion that I think can improve readability a little bit
else do | ||
-- check if the balance is positive or negative | ||
-- in one case we can produce change, in the other the inputs are insufficient | ||
balanceCheck sbe pp change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this check outside the if
, even though I understand it is equivalent (since the balanceCheck
already has an exception for this). But the code would be slightly simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I think I'll do a single check for the txout emptiness in the balance check only i.e. I'll change the output type to:
balanceCheck :: ... -> Either (TxBodyErrorAutoBalance era) IsEmpty
data IsEmpty = Empty | NonEmpty
3487606
to
266eb04
Compare
Changelog
Context
Previously for txout with 0 lovelace and some non-ada assets the returned error was about negative balance, which doesn't make sense.
Checklist