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

Auto-balance multiasset transactions #4450

Merged
merged 1 commit into from Apr 2, 2023

Conversation

LudvikGalois
Copy link
Contributor

@LudvikGalois LudvikGalois commented Sep 15, 2022

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

@LudvikGalois LudvikGalois force-pushed the ludvikgalois/autobalance-multiasset branch 2 times, most recently from 19a2154 to 71602cb Compare September 15, 2022 07:54
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a 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

@dorin100 dorin100 mentioned this pull request Sep 19, 2022
15 tasks
@CarlosLopezDeLara CarlosLopezDeLara linked an issue Sep 19, 2022 that may be closed by this pull request
15 tasks
@LudvikGalois LudvikGalois force-pushed the ludvikgalois/autobalance-multiasset branch from 71602cb to 3527085 Compare September 21, 2022 00:04
@LudvikGalois LudvikGalois marked this pull request as draft September 21, 2022 00:06
@Jimbo4350 Jimbo4350 force-pushed the ludvikgalois/autobalance-multiasset branch from 3527085 to 4893c10 Compare September 28, 2022 08:48
@Jimbo4350 Jimbo4350 marked this pull request as ready for review September 28, 2022 08:48
@LudvikGalois LudvikGalois force-pushed the ludvikgalois/autobalance-multiasset branch 2 times, most recently from 667e1b0 to 5aae12b Compare October 4, 2022 09:05

let changeTxOut = case multiAssetSupportedInEra cardanoEra of
Left _ -> lovelaceToTxOutValue $ Lovelace (2^(64 :: Integer)) - 1
Right multiAsset -> TxOutValue multiAsset (lovelaceToValue (Lovelace (2^(64 :: Integer)) - 1) <> nonAdaChange)

Choose a reason for hiding this comment

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

Hi @LudvikGalois

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.

Copy link
Contributor Author

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.

@saratomaz
Copy link

saratomaz commented Oct 24, 2022

I am getting an error when I tried to mint a token (the same test works on latest master):

cardano-cli transaction build 
--mint-script-file test_minting_and_burning_witnesses_ci0_urn_asset_name_True_multisig.script 
--tx-in 898f28715c1fd838f346c523a6977809c5f9726a54ed4dd3c27418c7a00f4463#0 
--tx-out addr_test1vqtp42zsxnuaqt4uw8xh565f8s7tfm98w88sakh9xp7fszg7tksef+3000000+5 11d47abca323a319956e447b7efea9abf91b943c05f2adc42375282a.636f75747473636f696e616d6573 
--mint 5 11d47abca323a319956e447b7efea9abf91b943c05f2adc42375282a.636f75747473636f696e616d6573 
--change-address addr_test1vqtp42zsxnuaqt4uw8xh565f8s7tfm98w88sakh9xp7fszg7tksef 
--witness-override 5 
--out-file test_minting_and_burning_witnesses_ci0_urn_asset_name_True_mint_tx.body 
--babbage-era 
--testnet-magic 42

Command failed: transaction build Error: Negative quantity (-5) in transaction output: TxOutInAnyEra BabbageEra (TxOut (AddressInEra (ShelleyAddressInEra ShelleyBasedEraBabbage) (ShelleyAddress Testnet (KeyHashObj (KeyHash "161aa85034f9d02ebc71cd7a6a893c3cb4eca771cf0edae5307c9809")) StakeRefNull)) (TxOutValue MultiAssetInBabbageEra (valueFromList [(AdaAssetId,18446744073709551615),(AssetId "11d47abca323a319956e447b7efea9abf91b943c05f2adc42375282a" "couttscoinames",-5)])) TxOutDatumNone ReferenceScriptNone)

@LudvikGalois LudvikGalois force-pushed the ludvikgalois/autobalance-multiasset branch from 5aae12b to 07fee52 Compare October 31, 2022 14:21
@newhoggy newhoggy force-pushed the ludvikgalois/autobalance-multiasset branch from 07fee52 to 3db046b Compare February 9, 2023 18:45
@newhoggy newhoggy self-assigned this Feb 28, 2023
@newhoggy newhoggy force-pushed the ludvikgalois/autobalance-multiasset branch from 3db046b to ae04ea5 Compare February 28, 2023 21:46
@newhoggy newhoggy force-pushed the ludvikgalois/autobalance-multiasset branch 2 times, most recently from 66e68f4 to 64391a6 Compare March 17, 2023 11:17
@Jimbo4350
Copy link
Contributor

@newhoggy was #4450 (comment) resolved?

@newhoggy newhoggy force-pushed the ludvikgalois/autobalance-multiasset branch from 64391a6 to daece12 Compare March 19, 2023 22:08
@newhoggy
Copy link
Contributor

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
@newhoggy newhoggy force-pushed the ludvikgalois/autobalance-multiasset branch from daece12 to b81ddeb Compare April 2, 2023 01:21
@newhoggy newhoggy enabled auto-merge April 2, 2023 01:21
@newhoggy newhoggy added this pull request to the merge queue Apr 2, 2023
Merged via the queue into master with commit 27f8437 Apr 2, 2023
21 of 23 checks passed
@iohk-bors iohk-bors bot deleted the ludvikgalois/autobalance-multiasset branch April 2, 2023 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants