-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix transaction autobalancing when deregistering credential #718
Fix transaction autobalancing when deregistering credential #718
Conversation
b70d25e
to
65f7e5b
Compare
LGTM. Ask me for approval when this is ready 👍 |
6577cbc
to
45c03e1
Compare
e0d4049
to
8642abb
Compare
8642abb
to
c6bba1b
Compare
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.
Can you add some more details as to exactly how you fixed this? It's not obvious to me what changes are the fix as they are overlapped with formatting changes.
c6bba1b
to
5416989
Compare
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.
Still unclear. Separate the formatting changes and the semantic changes with I understand the change now after reading the PR description. However we need to confirm that the change value does not influence the execution units calculation. I think it may as a larger change value means a larger script context. Confirm this with the plutus team.git add --patch
12a4f6d
to
3e12ec8
Compare
23ebe4f
to
a26c2c7
Compare
:: ShelleyBasedEra era -> Value -> TxBodyContent build era -> Value | ||
calculateChangeValue sbe incoming txbodycontent = | ||
let outgoing = calculateCreatedUTOValue sbe txbodycontent | ||
-- | Calculate the partial change - this does not include certificates' deposits |
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.
👍
Add a test case for that.
a26c2c7
to
63651cd
Compare
Changelog
Context
The first step of autobalancing was not considering ADA inflow from deregistering a credential, so the resulting change could get negative and it was blowing up in ledger code.
Detailed cause explanation
The negative intermediate change value could be a result of the first call to:
calculateChangeValue
considers only:so if there's not enough input value, meaning
tx outs > tx ins
, the calculated intermediate change gets negative. Note, that's a valid scenario if enough funds are coming in from certificate deposit return.If that change is then used as a new transaction output (the change output), it will throw an exception in the ledger code.
The fix
We're using the exact change value in the first step of autobalancing, for the execution units calculation.
Regression tests executed in:
Checklist