RoundRobin multi-asset coin-selection integration in cardano-wallet

[KtorZ]

Issue Number

ADP-506 / ADP-605

Overview

• 395b954
  📍 remove migration selection handlers until rework
  Most of the code doesn't apply anymore because of the way we've changed how coin-selection works. So I've simply thrown everything away. It'll be possible to look at git later when re-implementing this to get some inspiration. It is also very likely that we may want to do things slightly differently, the problem and context being now different.

• bc3d0a6
  📍 rework 'TransactionLayer' to play well with RoundRobin MA selection algs.
  The main change compared to before is the unification of all 'mkStdTx', 'mkDelegationJoinTx' and 'mkDelegationQuitTx' under one single interface 'mkTransaction'. We know for sure that this is possible because that's exactly how things are implemented behind the scene in the cardano-wallet package. This is a quite disruptive change, but should help really help simplifying the coin selection code which has grown large and complex over the past months with many pitfalls and logic duplication here and there.

  Another major change is the introduction of a 'TransactionCtx' type which should help unifying the caller interface in a more elegant way, instead of having to hand-craft every single argument by hand depending on the function to call. The main idea behind this 'TransactionCtx' is that it contains details about the transaction that are known prior to constructing the transaction. Typically, they'd come from the surrounding context (e.g. the current time) or, be user-provided.

• 84465c5
  📍 parameterize 'SelectionResult' over the change's inner type
  The reason for this is to be able to re-use the 'SelectionResult' at various stages in the wallet layer, before and after having assigned change address to each change output.
  It opens room for writing nice functions such as: 'assignChangeAddress :: SelectionResult TokenBundle -> s -> (s, SelectionResult TxOut)' which, from its type signature informs
  nicely about what it is doing.

• fc7260b
  📍 add 'extraCoinSource' to 'SelectionResult'
  So that we can fully construct transaction metadata from a 'SelectionResult' and need not to re-do any extra computation or pass extra arguments about withdrawals and reclaims.

• 0a99a26
  📍 add outputsCovered to 'performSelection', to make it possible to fully reconstruct a tx from a result
  Adding it as a list to cope with delegation selections which have no outputs.

• 09b9b14
  📍 decouple deposits from 'estimateFee'
  Fees and deposits are closely related but that aren't the same thing. So it's better to keep the fee estimation about fees, and leave deposits out of it.

• 8486593
  📍 integrate RoundRobin Multi-Asset selection and new transaction layer in the wallet layer

• 1a48de2
  📍 upgrade the server handlers to use the upgraded multi-asset wallet layer.

• b1a09c4
  📍 add simple conversion function to '../Types/Coin.hs' from Coin to integrals

• 524d5b5
  📍 move 'calcSelectionDelta' to '.../MA/RoundRobin' and make it work for all 'SelectionResult' types
  We need this in two places, on 'SeletionResult TxOut' and 'SelectionResult TokenBundle'.

• 7e49ff1
  📍 Upgrade cardano-wallet package to work with new TransactionLayer

• fda6581
  📍 Adjust error messages assertions in integration tests
  To match new errors generated from the multi-assets selection.

• 2eb1e7b
  📍 return and catch required cost with 'UnableToConstructChangeError'
  So that we can still return a value when users want to estimate fees but there's not enough money to run a full selection. Though, what we really need is a complete rework of the how transaction are created which does not rely on fee estimation.

• 8fc5683
  📍 run 'coinSelector' last in the multi-asset selection.

• aee4f87
  📍 implement pure version of 'selectionToUnsignedTx' for external coin selections
  Did rework a bit how 'selectAssets' also work and allowed callers to pass an extra transformation function on the result as argument, using the state with which the selection was calculated. This allows for callers to run some extra computations on the exact same state. We have two use-cases for this particular argument:

  • Fee estimation functions, which transform the selection result into a 'Coin'
  • So-called external selections ran for read-only wallets, which transform the selection into an 'UnsignedTx' with fully qualified (i.e. with derivation paths) inputs and change.

• 61c4872
  📍 Update types in RoundRobinSpec test suite.
  This change updates various types within the RoundRobinSpec test suite in
  light of recent changes to the RoundRobin module, allowing it to compile.

• 6c595e9
  📍 Test the value of outputsCovered returned by performSelection.
  In particular, we wish to ensure the following property holds:

outputsCovered == outputsToCover

• 2650cff
  📍 remove now obsolete coin-selection modules.

• b34a575
  📍 rename 'missingCoins' field to 'shortfall' in the MA selection change error

• 5264ba9
  📍 introduce 'defaultTransactionCtx' to avoid repetition of empty contexts in various situation

• e6319b1
  📍 update benchmarks & shelley transaction specs to work with new transaction layer.

• 6080b02
  📍 do not count external withdrawals as 'ours' when constructing tx metadata

• 6462940
  📍 make sure to count user-specified outputs that are ours when constructing tx meta

• 3ff070f
  📍 finish implementation of 'calcMinimumCoinValue' to also account for multi-assets

• ebde060
  📍 fix stake pool tests and transaction metadata reporting

• 581309d
  📍 remove now obsolete 'TransactionSpecShared' module.

• a606ca9
  📍 make 'estimateMaxNumberOfInputs' aware of multi-assets outputs.
  And adjust unit tests accordingly.

• 9b9bd2f
  📍 use current mainnet value as tx max size in unit test
  So that we get some order good of magnitudes when it comes to what can be expected for Mainnet.

• 971cd17
  📍 remove integration coin selection scenarios that are no-longer applicable.
  Indeed, the wallet now always create a change output. The only way
  to empty a wallet is to use the migration / swapping endpoints.

• 805e952
  📍 temporarily disable test integration migration scenarios
  Until the endpoints handlers are re-implemented.

Comments

Known issues / remaining work so far:

• 1. Transactions with withdrawals from an external wallet are wrongly labeled as "Incoming".

• 2. calcMinUTxOValue needs to be properly implemented (see PR Add function computeMinimumAdaQuantity. #2461)

• 3. Some picky tests regarding stake pools which work from very precise fee amounts needs to be updated because expected fees are no longer matching.

• 4. estimateMaxNumberOfInputs only consider ada-only assets, resulting in estimation that can be way above the real one. Needs to account for the actual output shape.

• 5. The equivalent of amount for multi-assets on transaction is currently always blank, instead of being assigned with the delta between inputs and outputs.

• 6. assets are missing from the external selections API, although the underlying code is already capable of running multi-asset external selections

• 7. assets are missing from the Byron API endpoints, whereas it is actually completely possible to have a Byron wallet with native assets.

• 8. Wallet migrations / swapping has been disabled

[jonathanknowles]

Hi @KtorZ

This is just a partial review, but wanted to submit it before heading off (as it's late over here!).

The only reservation I have so far relates to the parameterisation of SelectionResult. I suspect there might be a simpler solution (see comments).

Looking forward to reviewing the rest!

Cheers!

[jonathanknowles]

👍🏻

Suggestion:

Suggested change

	-- NOTE: We run the 'coinSelector' last, because we know that there are
	-- necessarily coins in all inputs. Therefore, after having ran the other
	-- selectors, we may already have covered for coins and need not to select
	-- extra inputs.
	-- NOTE: We run the 'coinSelector' last, because we know that every input
	-- necessarily has a non-zero ada amount. By running the other selectors
	-- first, we increase the probability that the coin selector will be able
	-- to terminate without needing to select an additional coin.

[rvl]

Looks good to me, nice clean ups with mkTransaction.

[rvl]

Looks like coinToNatural isn't used in this PR?
coinToInteger is used for difference - could it be replaced with a difference function?

[KtorZ]

Yes indeed, it used to be used before your PR which changed many of the Quantity to Coin. The coinToInteger is only used in the readWithdrawal function; Integer are used here because the difference may actually underflow / be negative. I'll see if that can be written differently using only distance or subtractCoin 👍

[Anviking]

Wondered a bit about why selectAssets is not in TransactionLayer rather than calcMinimumCoinValue, initSelectionCritereia, calcMinimumValue.

It seems readNextWithdrawal needs separate access to calcMinimumCost. It is used from the Server.hs module, but the result is directly put back into TransactionCtx.

I suspect that logic could fit in initSelectionCriteria, such that Wallet.hs and Server.hs don't need to know about it.

[KtorZ]

Wondered a bit about why selectAssets is not in TransactionLayer

The main reason is because selectAssets contains the glue code for pulling information from the database and running the selection itself. The TransactionLayer is completely agnostic to the database. So, one could argue that it suffices to make the UTxO and protocol parameters arguments of the selectAssets function to make it work; but that is basically what the performSelection function is already. Moreover, we'll still need to put the glue / effectful code somewhere to prepare the arguments.

Beside, having separate functions as calcMinimumCoinValue, initSelectionCriteria and calcMinimumValue allows in principle for easier testing since these functions cover a very specific sub-part of the logic.

It seems readNextWithdrawal needs separate access to calcMinimumCost. It is used from the Server.hs module, but the result is directly put back into TransactionCtx. I suspect that logic could fit in initSelectionCriteria, such that Wallet.hs and Server.hs don't need to know about it.

I am afraid that this won't work because the users are actually driving the request for withdrawal, not the server. This is why the server needs to know about it, because it depends on what the user has chosen. If we were systematically withdrawing from the account without asking the user, I think we could indeed move that logic to initSelectionCriteria. Yet at the moment, withdrawals are only set in the TransactionCtx if:

a) They are large enough
b) They have been explicitly requested by users.

[KtorZ]

cc @piotr-iohk This was also somewhat plain wrong. At least, now it's fixed.

[KtorZ]

Note: a little downside of the new unified approach for coin selection is that the algorithm must always be able to create a change output. Which means that, even is the wallet contains just enough to pay for fees and deposit, one would still get an error if there's not enough to cover for the min utxo value of the change output. The actual cost of the transaction is unchanged however, because the change goes back to the wallet.

[KtorZ]

cc @piotr-iohk

[Anviking]

Isn't this weird?

I guess neither label make perfect sense, but Outgoing more so?

[Anviking]

Or are we getting the deposit back now?

[KtorZ]

We do want Incoming because of the deposit indeed, there's more coming in than out, and that's how we define Incoming

[KtorZ]

Whoops. I was maybe a bit too fast in removing this one thinking it was something else. We likely still want something similar.

[KtorZ]

bors try

[iohk-bors]

try

Build failed:

• ci/hydra-build:required

#expected

[KtorZ]

bors try

[KtorZ]

bors r-

[KtorZ]

bors r+

[KtorZ]

bors r-

[iohk-bors]

Canceled.

[KtorZ]

bors try

ffs.

[KtorZ]

bors r+

[iohk-bors]

Build succeeded:

• buildkite/cardano-wallet
• ci/hydra-build:required