Reuse balance transaction in construct transaction #3553

paweljakubas · 2022-10-26T18:00:46Z

Added PreSelection along side SelectionOf
Enabled mkUnsignedTx to use either PreSelection or SelectionOf
constructTransaction produces "no-inputs/no-fee" sealed tx using mkUnsignedTx
constructTransaction uses balanceTransaction to add inputs and fees
coin selection and fee are retrieved from decodeTransaction
checking all integration tests
adding dummyTxIn as node does not allow producing no-inputs txs at the moment

Comments

Issue Number

adp-2257

jonathanknowles · 2022-11-01T06:33:34Z

Hi @paweljakubas!

EDIT: I've rebased the branch on master and fixed the warnings (for now).

It seems this branch currently doesn't build:

https://buildkite.com/input-output-hk/cardano-wallet/builds/22055#01841a4d-940e-4db4-a23b-03e048a26afa

There's also a (small) conflict with master. Perhaps we could kill two birds with one stone here: Would you be able to rebase the branch on top of master, and fix the build errors and warnings? 🙏🏻

Many thanks!

Jonathan

jonathanknowles · 2022-11-02T07:56:42Z

bors try

iohk-bors · 2022-11-02T09:01:48Z

try

Build failed:

ci/hydra-build:required

Failed as expected with balance_tx_zero_ada_output:

         , Left
             ( ClientError
                 ( Object
                     ( fromList
                         [
                             ( "code"
                             , String "balance_tx_zero_ada_output"
                             )
                         ,
                             ( "message"
                             , String "I don't currently support balancing transactions containing one or more zero-ada outputs. In the future I might be able to increase the values to the minimum allowed ada value."
                             )
                         ]
                     )
                 )
             )
         )
       expected: Status {
                   statusCode = 202,
                   statusMessage = "Accepted"
                 }
        but got: Status {
                   statusCode = 501,
                   statusMessage = "Not Implemented"
                 }

  To rerun use: --match "/API Specifications/NEW_SHELLEY_TRANSACTIONS/TRANS_NEW_CREATE_10h - Minting assets without timelock to foreign address/"

After the work for ADP-2347 (Support increasing zero-ada outputs to minimum in balanceTx) has been merged into master, we can rebase this PR on top of that.

jonathanknowles · 2022-11-03T06:31:13Z

lib/wallet/src/Cardano/Wallet.hs

@@ -2534,7 +2534,7 @@ constructTransaction
 -> WalletId
 -> Cardano.AnyCardanoEra
 -> TransactionCtx
- -> SelectionOf TxOut
+ -> Either PreSelection (SelectionOf TxOut)


Hi @paweljakubas

I'm a bit confused about why this argument needs to be of type Either PreSelection Selection.

Is there a good reason for this? (Perhaps preparing for the future?)

AFAICT, this function is only ever called with Left PreSelection, so we could just simplify the argument type to PreSelection.

See the following commit, which still compiles: 262b7b9.

hi @jonathanknowles indeed. PreSelection was introduced as the intention was to construct tx WITHOUT inputs and SelectionOf has NonEmpty of inputs. But when I proceeded then it turn out that node does not accept constructing tx with empty inputs. Hence this dummyInput hack. But, node intends to lift this restriction in coming version of node, so I intended to stick to differentiating PreSelection and SelectionOf.

lib/wallet/src/Cardano/Wallet/CoinSelection.hs

Anviking · 2022-11-08T19:09:21Z

bors try

iohk-bors · 2022-11-08T21:13:53Z

try

Build failed:

ci/hydra-build:required

iohk-bors · 2022-11-10T12:03:48Z

👎 Rejected by too few approved reviews

Unisay · 2022-11-14T18:18:17Z

I am taking over this branch

Use `LambdaCase` instead of repeating patterns. Use `Coin.fromQuantity` instead of `fromIntegral`.

This removes the incomplete pattern match warnings.

The function `Cardano.Wallet.constructTransaction` is only ever called with a `Left PreSelection`, and never called with a `Right Selection`. Therefore, we can simplify the selection argument type from `Either PreSelection Selection` to just `PreSelection`.

This helper type is only used by the `constructTransaction` and `mkUnsignedTx` functions, and not used by anything within the `CoinSelection` module hierarchy.

Without this change integration tests would fail e.g. where: ```pseudo-code constructTx paymentTo addr1 (0 ada) | | V Tx output: addr1 (1 ada) -- min ada, set automatically output: addr2 (9 ada) -- change ``` as we'd believe both outputs in the resuling tx were change -- neither of them exists in the original output list. If there are 'n' outputs in the original unbalanced tx, we will with this commit instead judge that the first 'n' are "outputs" and that the ones after that are all change. While this makes assumptions about how balanceTx appends outputs, it is more importantly ressilient to outputs being modified, like with the `increaseZeroAdaOutputs` feature.

Unisay · 2022-11-21T18:37:16Z

bors merge

iohk-bors · 2022-11-21T19:48:09Z

Build succeeded:

…ction r=Unisay a=paweljakubas  - [x] Added PreSelection along side SelectionOf - [x] Enabled mkUnsignedTx to use either PreSelection or SelectionOf - [x] constructTransaction produces "no-inputs/no-fee" sealed tx using mkUnsignedTx - [x] constructTransaction uses balanceTransaction to add inputs and fees - [x] coin selection and fee are retrieved from decodeTransaction - [x] checking all integration tests - [x] adding dummyTxIn as node does not allow producing no-inputs txs at the moment ### Comments  ### Issue Number adp-2257  Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Yura Lazarev <[email protected]> Source commit: 2363059

paweljakubas requested review from jonathanknowles and Anviking October 26, 2022 18:00

paweljakubas self-assigned this Oct 26, 2022

paweljakubas changed the title ~~Paweljakubas/adp 2257/reuse balance transaction in construct transaction~~ Reuse balance transaction in construct transaction Oct 26, 2022

paweljakubas marked this pull request as ready for review October 28, 2022 13:24

paweljakubas mentioned this pull request Oct 28, 2022

Reuse balance transaction in construct shared transaction #3547

Closed

5 tasks

jonathanknowles force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch 2 times, most recently from 615178a to 92de589 Compare November 2, 2022 07:26

iohk-bors bot added a commit that referenced this pull request Nov 2, 2022

Try #3553:

d3568b3

jonathanknowles force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch 2 times, most recently from 8598029 to 7226abe Compare November 3, 2022 05:54

jonathanknowles mentioned this pull request Nov 3, 2022

Review PR #3553 #3567

Closed

jonathanknowles reviewed Nov 3, 2022

View reviewed changes

Anviking reviewed Nov 3, 2022

View reviewed changes

lib/wallet/src/Cardano/Wallet/CoinSelection.hs Outdated Show resolved Hide resolved

Anviking force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch from f154f16 to eb72775 Compare November 8, 2022 19:02

Anviking changed the base branch from master to anviking/ADP-2347/balanceTx-minAda November 8, 2022 19:03

iohk-bors bot added a commit that referenced this pull request Nov 8, 2022

Try #3553:

d441e88

Unisay force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch from eb72775 to 6b1f160 Compare November 10, 2022 11:02

Anviking force-pushed the anviking/ADP-2347/balanceTx-minAda branch 3 times, most recently from d9a6ee6 to 86448bd Compare November 14, 2022 14:54

Unisay force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch from 6b1f160 to ab4539e Compare November 15, 2022 08:13

Anviking force-pushed the anviking/ADP-2347/balanceTx-minAda branch from e903498 to b38c399 Compare November 15, 2022 11:33

Anviking and others added 22 commits November 21, 2022 15:48

Make contents of dummyInput stand out more

5d45589

Remove dummy input immediately after calling makeTransactionBody

530b0d6

adding minting script witness to estimateNumberOfWitnesses

c843c06

be precise about number of minting scripts

7e70871

update test of withdrawal from self, 0 rewards

aa61a6d

Fix imports.

19101f5

Miscellaneous formatting fixes.

1001601

Make inner functions of constructTransaction more concise.

45e344f

Use `LambdaCase` instead of repeating patterns. Use `Coin.fromQuantity` instead of `fromIntegral`.

Factor out common build command within definition of inputWits.

d32f3b6

Remove redundant IsCardanoEra constraint.

34fcc5e

Match all cases in function removeDummyInput.

e5d9c95

This removes the incomplete pattern match warnings.

Add reminder to remove the dummyInput hack.

4bdee5a

Move type PreSelection to Cardano.Wallet.Transaction.

cd4314f

This helper type is only used by the `constructTransaction` and `mkUnsignedTx` functions, and not used by anything within the `CoinSelection` module hierarchy.

refactor: explicit parameters for the Wallet.constructTransaction

8be3361

chore: code formatting

ba55d2c

chore: improve comment on PreSelection data type

ddaabb9

fix: removeDummyInput does nothing for non-babbage txs.

d5b73df

feat: remove a dummy input from a Mary tx body

9685c5c

Safer dummy input removal

96c467b

refactor: better structured validation logic in the constructTransaction

6473ee4

Unisay force-pushed the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch from 7e7a60b to 6473ee4 Compare November 21, 2022 15:39

iohk-bors bot merged commit 2363059 into master Nov 21, 2022

iohk-bors bot deleted the paweljakubas/adp-2257/reuse-balance-transaction-in-construct-transaction branch November 21, 2022 19:48

This was referenced Nov 23, 2022

Cardano Wallet unable to prioritise the selection of larger UTxOs when transaction space is limited. #3541

Open

Extract DelegationRequest data type and use in in the constructTransaction #3602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse balance transaction in construct transaction #3553

Reuse balance transaction in construct transaction #3553

paweljakubas commented Oct 26, 2022 •

edited by jonathanknowles

Loading

jonathanknowles commented Nov 1, 2022 •

edited

Loading

jonathanknowles commented Nov 2, 2022

iohk-bors bot commented Nov 2, 2022 •

edited by jonathanknowles

Loading

jonathanknowles Nov 3, 2022 •

edited

Loading

paweljakubas Nov 3, 2022

Anviking commented Nov 8, 2022

iohk-bors bot commented Nov 8, 2022

iohk-bors bot commented Nov 10, 2022

Unisay commented Nov 14, 2022

Unisay commented Nov 21, 2022

iohk-bors bot commented Nov 21, 2022

Reuse balance transaction in construct transaction #3553

Reuse balance transaction in construct transaction #3553

Conversation

paweljakubas commented Oct 26, 2022 • edited by jonathanknowles Loading

Comments

Issue Number

jonathanknowles commented Nov 1, 2022 • edited Loading

jonathanknowles commented Nov 2, 2022

iohk-bors bot commented Nov 2, 2022 • edited by jonathanknowles Loading

try

jonathanknowles Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

paweljakubas Nov 3, 2022

Choose a reason for hiding this comment

Anviking commented Nov 8, 2022

iohk-bors bot commented Nov 8, 2022

try

iohk-bors bot commented Nov 10, 2022

Unisay commented Nov 14, 2022

Unisay commented Nov 21, 2022

iohk-bors bot commented Nov 21, 2022

paweljakubas commented Oct 26, 2022 •

edited by jonathanknowles

Loading

jonathanknowles commented Nov 1, 2022 •

edited

Loading

iohk-bors bot commented Nov 2, 2022 •

edited by jonathanknowles

Loading

jonathanknowles Nov 3, 2022 •

edited

Loading