-
Notifications
You must be signed in to change notification settings - Fork 720
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
Auto-balance multiasset transactions #4450
Conversation
19a2154
to
71602cb
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.
LGTM! Liaise with QA and produce a ticket like discussed in the node team call. Only bors the PR when QA has signed off on it
71602cb
to
3527085
Compare
3527085
to
4893c10
Compare
667e1b0
to
5aae12b
Compare
|
||
let changeTxOut = case multiAssetSupportedInEra cardanoEra of | ||
Left _ -> lovelaceToTxOutValue $ Lovelace (2^(64 :: Integer)) - 1 | ||
Right multiAsset -> TxOutValue multiAsset (lovelaceToValue (Lovelace (2^(64 :: Integer)) - 1) <> nonAdaChange) |
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.
Just curious: do you have any expectations about how the makeTransactionBodyAutoBalance
function should behave if the changeTxOut
value is larger that the maximum value size? (As defined by the maxValueSize
protocol parameter.)
In cardano-wallet
, if we detect that a generated (change) output is larger than the maximum size, we partition it into a number of smaller outputs.
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.
It's not currently handled here, although it'll be caught when trying to submit. My preference for now is to "give up" and require the user to explicitly make change themselves when building the transaction (my understanding is that this is an edge case in most cases). Manually balancing multiassets is much easier for a human (or script) to do than balancing Ada, because it doesn't need to account for some of what its balancing disappearing as transaction fees.
Multiple change UTxOs is straightforward, but given that we don't know how the user plans to spend it, we might sub-optimally divide it, which could end up costing the user more in fees.
I am getting an error when I tried to mint a token (the same test works on latest master):
|
5aae12b
to
07fee52
Compare
07fee52
to
3db046b
Compare
3db046b
to
ae04ea5
Compare
66e68f4
to
64391a6
Compare
@newhoggy was #4450 (comment) resolved? |
64391a6
to
daece12
Compare
Previously we gave up when the non-Ada part of a transaction wasn't balanced. We now balance the transaction and correctly update the fee accordingly (since the fee will be higher). We also return an error in the case where the is non-Ada change, but not at least minUTxO change (e.g. in the case where the Ada is already balanced). Resolves: #3068
daece12
to
b81ddeb
Compare
Previously we gave up when the non-Ada part of a transaction wasn't balanced. We now balance the transaction and correctly update the fee accordingly (since the fee will be higher). We also return an error in the case where the is non-Ada change, but not at least minUTxO change (e.g. in the case where the Ada is already balanced).
Resolves: #3068
Closes: #4453