Balance endpoint integration #2906

paweljakubas · 2021-09-16T16:23:49Z

We have preliminary impl of balanceTransaction that needs to work:

coin selection with external inputs (@jonathanknowles ) DONE
updateTx impl in shelley (@Anviking ) DONE

running tests:

stack test --ta '-m "TRANS_NEW_BALANCE_"' cardano-wallet:integration

Comments

Issue Number

2914: Add updateSealedTx and ExtraTxBodyContent r=Anviking a=Anviking - [x] Refactoring: Replace `to{Shelley, Allegra, Mary, Alonzo}TxOut` with `toCardanoTxOut` for easier re-use. - [x] Add `updateSealedTx :: SealedTx -> ExtraTxBodyContent -> Either ErrUpdateSealedTx SealedTx` - [x] Test that `updateSealedTx noExtraBodyContent` works with all PAB goldens - [x] Test that `updateSealedTx noExtraBodyContent` fails when there are existing key witnesses - [x] Test that inputs and outputs are combined correctly. ### Comments Not tested: - That it works for all previous eras.  ### Issue Number ADP-1140 Relates to / should be needed by #2906  Co-authored-by: Johannes Lund <[email protected]>

jonathanknowles · 2021-10-01T12:13:19Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+                UTxOIndex.fromSequence externalInputs
+
+        let utxoAvailable = UTxOSelection.fromIndexPair
+                (internalUtxoAvailable, externalSelectedUtxo)


This looks good. 👍🏻

Anviking · 2021-10-01T13:24:47Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+
+    let UnsignedTx colls inps _outs change _wdrl = unsignedtx
+    let extraBody = ExtraTxBodyContent
+            { extraInputs = (\(txin, _, _) -> txin) <$> F.toList inps


Won't inps already include the pre-selected inputs from sealedTxIncoming?

updateTx would then try to append the whole inps to the pre-selected inputs.

Although as the ledger represents the inputs as a Set, it might not actually matter in the end.

yes, we may filter out preselected inputs. although ledger stores inputs as set so it should not affect txbody or fee.

jonathanknowles · 2021-10-04T08:34:21Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+        (wdrl, _) <-
+            if Map.member acct wdrlMap then
+                mkRewardAccountBuilder @_ @s @_ @n ctx wid (Just SelfWithdrawal)
+            else
+                mkRewardAccountBuilder @_ @s @_ @n ctx wid Nothing


Suggested change

(wdrl, _) <-

if Map.member acct wdrlMap then

mkRewardAccountBuilder @_ @s @_ @n ctx wid (Just SelfWithdrawal)

else

mkRewardAccountBuilder @_ @s @_ @n ctx wid Nothing

(wdrl, _) <- mkRewardAccountBuilder @_ @s @_ @n ctx wid $

if Map.member acct wdrlMap

then Just SelfWithdrawal

else Nothing

jonathanknowles · 2021-10-04T08:36:18Z

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs

+        -- *However* when used to respresent the inputs known by the wallet, in
+        -- contrast to all inputs, it can be empty.


Suggested change

-- *However* when used to respresent the inputs known by the wallet, in

-- contrast to all inputs, it can be empty.

-- *However* when used to represent the inputs known by the wallet, in

-- contrast to all inputs, it can be empty.

Anviking · 2021-10-04T08:47:07Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+        let transform s sel' =
+                W.assignChangeAddresses genChange sel' s
+                & uncurry (W.selectionToUnsignedTx (txWithdrawal txCtx))
+        unsignedtx <- liftHandler $ W.selectAssets @_ @s @k wrk W.SelectAssetsParams


Because selectAssets only knows about the things we tell it, and not the entire sealedTxIncoming, I think exotic things like submitting update proposals will result in imbalanced txs.

In a re-thought approach, related to ADP-891, and I think which you've mentioned, we could calculate minfee sealedTxIncoming instead and go from there (either by overestimating for the TxExtraBodyContent, or trying to find some fix point)

But I guess the similar and more pressing issue is datums, which everyone was aware of on today's call.

agree here, it is reasonable to start with minFee, and then increment knowing the cost per inp/out/coll. But we need to remember that evaluation of min fee requires number of witnesses, so it would influence selecting inputs/collaterals in feedback loop. It is not staightforward for me how, in details, it should work

Anviking · 2021-10-04T11:10:44Z

lib/core/src/Cardano/Wallet.hs

@@ -1346,28 +1346,24 @@ selectionToUnsignedTx wdrl sel s =
            fullyQualifiedWithdrawal wdrl
        }
  where
+    -- NOTE: External addresses, not known to the wallet, will be filtered out.


When talking briefly with Nikola and hearing / being reminded that they will present the result of balanceTransaction in full detail to the user, this seems wrong.

We probably should:

Make the DerivationPath optional on ApiCoinSelection

Don't filter out pre-selected inputs/outputs

Ensure all other possible tx data is exposed. (We're probably missing glue code for showing datums in api here)

(Not critical for merging this PR and I could have a look)

Daedalus will invoke balance tx, so it will know details of external inputs already - see "inputs" in https://input-output-hk.github.io/cardano-wallet/api/edge/#operation/balanceTransaction And in ApiCoinSelection we will present ADDED inps/outs/colls (always being the resource of the wallet). So the client will know everything having "inputs" and response.
Some argument in favor of what is now is that it is in line what constructTx responds, and as both construct and balance are first step before sign+submit. It makes sense to be compatible....

Anviking

It is great to see this start coming together concretely!

My take is that remaining problems are:

Critical: Ensure fees are not underestimated because of datums
Critical: Ensure e.g. ProtocolParameter Hash, or other stuff won't lead to underestimated fees
Moderate: Is the API response for CoinSelection appropriate for Daedalus?
Low/Moderate: Certificates, update proposals leading to underestimated fees

and that we shouldn't rush to merge this before seeing how the critical things will pan out.

I suspect there are 3 potential solutions for (1):
a. Tweak the coin selection in a minimal / hacky way to account for datums
b. Call ledger/node's minfee in a loop - first on the partial input tx, and then after adding inputs and outputs until you converge.
c. Call ledger/node's minfee once, and then use the existing TxConstraints overestimations to add additional inputs and outputs.

Edit

I must have gotten confused in the original message. Got things a bit wrong. Now edited. We are adjusting the original binary. Ensuring tx body content is preserved is not the problem. The problem is ensuring the fee estimation also is aware of all that content.

Anviking · 2021-10-04T13:35:41Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+        (ApiTxOut (ApiT addr, _) _ (Quantity amt) (ApiT assets))) =
+        (TxIn hashTx ix, TxOut addr (TokenBundle (Coin $ fromIntegral amt) assets))
+    externalInputs = toTxInTxOut <$> apiExternalInps
+    sealedTxIncoming = body ^. #transaction . #getApiT


Just some stylistic thoughts:
I think I have more of a taste for either:

Defining important values as let bindings at the top

Making the where definitions all functions rather than values

such that you can read it from top to bottom and easily see where txIncoming is coming from.

Anviking · 2021-10-04T13:39:00Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+balanceTransaction ctx genChange (ApiT wid) body = do
+    pp <- liftIO $ NW.currentProtocolParameters nl
+
+    --TODO  deal with coll and validity and delegations


👍 Yes.

Collateral we could simply forbid as discussed.

Delegations and other stuff could be present in the sealedTxIncoming, but are probably not crucial. (unused by PAB)

Validity, however, does sound crucial to support. (used by PAB)

Related to #2906 (comment)

🤔 might have gotten this slightly wrong. Will revisit tomorrow.

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

paweljakubas · 2021-10-05T12:19:19Z

bors r+

@jonathanknowles

2906: Balance endpoint integration r=paweljakubas a=paweljakubas  adp-656 We have preliminary impl of balanceTransaction that needs to work: 1. coin selection with external inputs (@jonathanknowles ) DONE 2. updateTx impl in shelley (@Anviking ) DONE running tests: ``` stack test --ta '-m "TRANS_NEW_BALANCE_"' cardano-wallet:integration ``` ### Comments  ### Issue Number  Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

hlint

For pre-selected external addresses, we cannot find derivation paths. So rather than erroring: ``` "selectionToUnsignedTx: unable to find derivation path of a known \ \input or change address. This is impossible.") (traverse withDerivationPath hasAddresses) ``` This commit filters them out. This has the effect of: 1. Making `stack test --ta '-m "TRANS_NEW_BALANCE_"' cardano-wallet:integration pass` 2. Removing the requirement that all ApiCoinSelections have a non-empty list of inputs.

some cleaning

iohk-bors · 2021-10-05T13:47:04Z

Canceled.

paweljakubas · 2021-10-05T13:47:27Z

bors r+

iohk-bors · 2021-10-05T15:35:53Z

Build succeeded:

paweljakubas requested review from jonathanknowles, Anviking, rvl and sevanspowell September 16, 2021 16:23

paweljakubas self-assigned this Sep 16, 2021

rvl mentioned this pull request Sep 17, 2021

Balance endpoint - to be retired #2782

Closed

10 tasks

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from e83a76f to 19e95df Compare September 20, 2021 09:07

Anviking mentioned this pull request Sep 20, 2021

Add updateSealedTx and ExtraTxBodyContent #2914

Merged

5 tasks

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch 6 times, most recently from c0165d1 to 386791f Compare October 1, 2021 12:09

jonathanknowles reviewed Oct 1, 2021

View reviewed changes

Anviking reviewed Oct 1, 2021

View reviewed changes

Anviking force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from 19cd20c to 5d0a3ea Compare October 1, 2021 17:03

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from 9a35f0e to 37bdb06 Compare October 1, 2021 18:16

paweljakubas requested review from jonathanknowles and Anviking October 1, 2021 18:22

jonathanknowles reviewed Oct 4, 2021

View reviewed changes

Anviking reviewed Oct 4, 2021

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from 37bdb06 to f93e437 Compare October 4, 2021 13:12

Anviking reviewed Oct 4, 2021

View reviewed changes

Anviking approved these changes Oct 5, 2021

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from fc3ea41 to 18be8f9 Compare October 5, 2021 12:19

paweljakubas and others added 12 commits October 5, 2021 15:46

obtain execution prices from network

0a50a28

add scaffolding for balanceTransaction to be filled

6ef358e

torwards using selectAssets in balanceTransaction

e230b66

change updateTx, more work on impl of balanceTransaction

11b595d

work on txCtx

c0fcfb6

use ttl and outputs from decodedTx

0ff5990

hlint

use properly coin selection with external inputs

95fbb58

enable updating with collaterals

2a21fa2

unpend the test

955933c

enable test 1d - normal tx with missing inputs

fb20446

some cleaning

some cleanings

ac30014

some cleaning

paweljakubas force-pushed the paweljakubas/adp-656/balance-endpoint-integration branch from 18be8f9 to ac30014 Compare October 5, 2021 13:47

iohk-bors bot merged commit 234687d into master Oct 5, 2021

iohk-bors bot deleted the paweljakubas/adp-656/balance-endpoint-integration branch October 5, 2021 15:35

WilliamKingNoel-Bot pushed a commit that referenced this pull request Oct 5, 2021

docs: Merge #2906

274d3a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Balance endpoint integration #2906

Balance endpoint integration #2906

paweljakubas commented Sep 16, 2021 •

edited

Loading

jonathanknowles Oct 1, 2021

Anviking Oct 1, 2021

paweljakubas Oct 1, 2021

jonathanknowles Oct 4, 2021

paweljakubas Oct 4, 2021

jonathanknowles Oct 4, 2021

Anviking Oct 4, 2021

paweljakubas Oct 4, 2021

Anviking Oct 4, 2021

Anviking Oct 4, 2021

paweljakubas Oct 4, 2021

Anviking left a comment •

edited

Loading

Anviking Oct 4, 2021 •

edited

Loading

Anviking Oct 4, 2021 •

edited

Loading

Anviking Oct 4, 2021

paweljakubas commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

paweljakubas commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

		-- However when used to respresent the inputs known by the wallet, in
		-- contrast to all inputs, it can be empty.

Balance endpoint integration #2906

Balance endpoint integration #2906

Conversation

paweljakubas commented Sep 16, 2021 • edited Loading

Comments

Issue Number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking left a comment • edited Loading

Choose a reason for hiding this comment

Edit

Anviking Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Anviking Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

paweljakubas commented Oct 5, 2021

iohk-bors bot commented Oct 5, 2021

paweljakubas commented Sep 16, 2021 •

edited

Loading

Anviking left a comment •

edited

Loading

Anviking Oct 4, 2021 •

edited

Loading

Anviking Oct 4, 2021 •

edited

Loading