Add derivation paths to coin selection change outputs #2244

jonathanknowles · 2020-10-15T03:24:35Z

Issue Number

#2247

Overview

This PR adjusts the API selectCoins operation so that:

ordinary outputs and change outputs are held in separate fields in the returned selection:

Field Description

outputs ordinary payments to external wallets

change change to be returned to the wallet
each change output now has a derivation_path field containing the full derivation path for the change output address.

The format of this field is identical to the format of the existing derivation_path field of each input.

Click here to see updated API description

Internal Changes

This PR introduces a ApiCoinSelectionChange type that includes a derivation path:

data ApiCoinSelectionChange (n :: NetworkDiscriminant) = ApiCoinSelectionChange
    { address :: !(ApiT Address, Proxy n)
    , amount :: !(Quantity "lovelace" Natural)
    , derivationPath :: NonEmpty (ApiT DerivationIndex)
    } deriving (Eq, Generic, Show)

In addition, we modify the ApiCoinSelection type so that ordinary outputs (without derivation paths) and change outputs (with derivation paths) are held in separate fields:

data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection
    { inputs :: !(NonEmpty (ApiCoinSelectionInput n))
-   , outputs :: !(NonEmpty (AddressAmount (ApiT Address, Proxy n)))
+   , outputs :: !(NonEmpty (ApiCoinSelectionOutput n))
+   , change :: ![ApiCoinSelectionChange n]
    } deriving (Eq, Generic, Show)

Testing

This PR updates the following integration tests:

WALLETS_COIN_SELECTION_01.
WALLETS_COIN_SELECTION_02.
SHELLEY_HW_WALLETS/HW_WALLETS_04.
BYRON_HW_WALLETS/HW_WALLETS_04.

paweljakubas · 2020-10-15T13:56:34Z

lib/core/src/Cardano/Wallet.hs

+    -- have been assigned with addresses and are included in the set of ordinary
+    -- outputs. We use the 'Void' type here to prevent callers from accidentally
+    -- passing change values into this function:
+    -> UnsignedTx input output Void


for quite a while have not seen Void :-)

Originally I wrote this by specifying change ~ Void in the list of constraints, and then specifying UnsignedTx input output change as the parameter type, as I felt that this would be more readable.

However, GHC views this as a redundant constraint. Of course, it's possible to silence the redundant constraint warning, but that would mean silencing this warning for the whole file. So I eventually switched to just writing Void inline, even though it is slightly less readable (IMO).

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

iohk-bors · 2020-10-16T07:26:00Z

try

Build succeeded:

paweljakubas

so far lgtm.having a way to derive change and inputs begs for some advanced testing sometime in future: we have a wallet with mnemonic1 with one utxo, and send some money to another one. We can now have a handle to output and change. We re-derive the addresses using derivation paths in cardano-addresses. And use them to create another transaction blob in cardano-transactions. Then send it using external api endpoint and see that it works in integration testing . Maybe @piotr-iohk would be interested in adding such a test sometime in the future:-)

This function is currently only used in contexts where all change outputs have been assigned with addresses and are included in the set of ordinary outputs. We use the 'Void' type here to prevent callers from accidentally passing change values into this function.

Use `Void` type in function `assignMigrationAddresses` to indicate that no change is returned.

Use this function to factor out common parts of `selectCoinsExternal`.

Before this change we were constructing payment addresses from the stake key, which remains constant over time. This resulted in the generation of a constant "payment address" that was not recognizable as a valid payment address belonging to the wallet, causing subsequent calls to `isOurs` to fail.

Update integration test `WALLETS_COIN_SELECTION_01` to check that: - change outputs are listed within the `change` field of an `ApiCoinSelection`. - change output derivation paths are valid.

Update integration test `WALLETS_COIN_SELECTION_02` to check that the set of coin selection outputs is exactly the same as intially provided, rather than being a subset.

Update integration test `SHELLEY_HW_WALLETS/HW_WALLETS_04` to make multiple payments. In addition, we expect that the resulting selection will have: * an output for each of the original payments. * at least one change output.

Update integration test `BYRON_HW_WALLETS/HW_WALLETS_04` to make multiple payments. In addition, we expect that the resulting selection will have: * an output for each of the original payments. * at least one change output.

We now expect that there are: * no ordinary outputs. * at least one change output.

jonathanknowles · 2020-10-21T05:39:37Z

bors try

jonathanknowles · 2020-10-21T06:55:27Z

bors r+

2244: Add derivation paths to coin selection change outputs r=jonathanknowles a=jonathanknowles # Issue Number #2247 # Overview This PR introduces a new `ApiCoinSelectionChange` type that includes a derivation path: ```hs data ApiCoinSelectionChange (n :: NetworkDiscriminant) = ApiCoinSelectionChange { address :: !(ApiT Address, Proxy n) , amount :: !(Quantity "lovelace" Natural) , derivationPath :: NonEmpty (ApiT DerivationIndex) } deriving (Eq, Generic, Show) ``` In addition, we modify the `ApiCoinSelection` type so that ordinary outputs (**_without_** derivation paths) and change outputs (**_with_** derivation paths) are held in separate fields: ```patch data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection { inputs :: !(NonEmpty (ApiCoinSelectionInput n)) - , outputs :: !(NonEmpty (AddressAmount (ApiT Address, Proxy n))) + , outputs :: !(NonEmpty (ApiCoinSelectionOutput n)) + , change :: ![ApiCoinSelectionChange n] } deriving (Eq, Generic, Show) ``` # Comments This PR updates the following integration tests: - `WALLETS_COIN_SELECTION_01`. - `WALLETS_COIN_SELECTION_02`. - `SHELLEY_HW_WALLETS/HW_WALLETS_04`. - `BYRON_HW_WALLETS/HW_WALLETS_04`. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2020-10-21T08:34:51Z

try

Timed out.

iohk-bors · 2020-10-21T09:04:51Z

Timed out.

#2279

jonathanknowles · 2020-10-21T09:19:22Z

bors r+

2244: Add derivation paths to coin selection change outputs r=jonathanknowles a=jonathanknowles # Issue Number #2247 # Overview This PR introduces a new `ApiCoinSelectionChange` type that includes a derivation path: ```hs data ApiCoinSelectionChange (n :: NetworkDiscriminant) = ApiCoinSelectionChange { address :: !(ApiT Address, Proxy n) , amount :: !(Quantity "lovelace" Natural) , derivationPath :: NonEmpty (ApiT DerivationIndex) } deriving (Eq, Generic, Show) ``` In addition, we modify the `ApiCoinSelection` type so that ordinary outputs (**_without_** derivation paths) and change outputs (**_with_** derivation paths) are held in separate fields: ```patch data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection { inputs :: !(NonEmpty (ApiCoinSelectionInput n)) - , outputs :: !(NonEmpty (AddressAmount (ApiT Address, Proxy n))) + , outputs :: !(NonEmpty (ApiCoinSelectionOutput n)) + , change :: ![ApiCoinSelectionChange n] } deriving (Eq, Generic, Show) ``` # Comments This PR updates the following integration tests: - `WALLETS_COIN_SELECTION_01`. - `WALLETS_COIN_SELECTION_02`. - `SHELLEY_HW_WALLETS/HW_WALLETS_04`. - `BYRON_HW_WALLETS/HW_WALLETS_04`. 2257: API data-types & specifications for arbitrary metadata signing. r=rvl a=KtorZ # Issue Number  ADP-481 > ADP-508 # Overview  - 209781b 📍 **define ToText/FromText instances for 'DerivationIndex'** I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime. - c965936 📍 **rename UTxO*ternal -> Utxo*ternal** We use the data-type constructor in order to generate text instances. However, our automatic casing function is doing pretty bad with the mixed cases and we end up with something like: - u_tx_o_external - u_tx_o_internal - mutable_account Instead of fixing the instance itself to work around this, I found it easier to just fix the type (which isn't used in many places). - c4cbafe 📍 **do not output back invalid input when parsing text values.** This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options. - fb5ab0d 📍 **extend API and specifications to include the 'signMetadata' operation** # Comments   Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-10-21T11:38:21Z

This PR was included in a batch that timed out, it will be automatically retried

iohk-bors · 2020-10-21T14:30:33Z

Build succeeded:

2261: Update the API specification to reflect the minimum number of inputs returned by `selectCoins`. r=KtorZ a=jonathanknowles # Issue Related to #2244. # Overview This PR updates the API specification for the `selectCoins` operation, changing the minimum number of inputs from `0` to `1` in the returned coin selection. This matches the existing `ApiCoinSelection` type, which specifies a `NonEmpty` list of `ApiCoinSelectionInput` values: ```hs data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection { inputs :: !(NonEmpty (ApiCoinSelectionInput n)) , outputs :: ![ApiCoinSelectionOutput n] , change :: ![ApiCoinSelectionChange n] , certificates :: Maybe (NonEmpty ApiCertificate) } deriving (Eq, Generic, Show) ``` Co-authored-by: Jonathan Knowles <[email protected]>

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch from e2d903e to d15c740 Compare October 15, 2020 03:58

iohk-bors bot added a commit that referenced this pull request Oct 15, 2020

Try #2244:

7741017

cardano-foundation deleted a comment from iohk-bors bot Oct 15, 2020

iohk-bors bot added a commit that referenced this pull request Oct 15, 2020

Try #2244:

b590adf

jonathanknowles mentioned this pull request Oct 15, 2020

Differentiate change outputs from standard outputs in coin selection #2247

Closed

jonathanknowles self-assigned this Oct 15, 2020

jonathanknowles requested a review from paweljakubas October 15, 2020 09:01

jonathanknowles added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Oct 15, 2020

This comment has been minimized.

Sign in to view

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch from f171225 to 070ebfc Compare October 15, 2020 13:52

paweljakubas reviewed Oct 15, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

iohk-bors bot added a commit that referenced this pull request Oct 15, 2020

Try #2244:

73ec1c9

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch from 070ebfc to 77a335f Compare October 16, 2020 06:04

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Oct 16, 2020

Try #2244:

de14862

paweljakubas approved these changes Oct 16, 2020

View reviewed changes

cardano-foundation deleted a comment from iohk-bors bot Oct 16, 2020

jonathanknowles changed the title ~~WIP: Add derivation paths to coin selection change outputs~~ Add derivation paths to coin selection change outputs Oct 16, 2020

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch from 77a335f to 431b2cd Compare October 16, 2020 09:12

cardano-foundation deleted a comment from iohk-bors bot Oct 16, 2020

jonathanknowles added the DO NOT MERGE label Oct 16, 2020

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch 7 times, most recently from d0dc17e to b991025 Compare October 19, 2020 10:30

jonathanknowles added 14 commits October 21, 2020 05:08

Use Void type in function assignMigrationAddresses.

0b11cd1

Use `Void` type in function `assignMigrationAddresses` to indicate that no change is returned.

Add function qualifyAddresses.

60111a8

Use this function to factor out common parts of `selectCoinsExternal`.

Move creation of unsigned TX into atomically block.

d625a84

Simplify error type synonyms.

9b55df3

Remove redundant constraints.

b9db49d

Update integration test WALLETS_COIN_SELECTION_01.

69ff27b

Update integration test `WALLETS_COIN_SELECTION_01` to check that: - change outputs are listed within the `change` field of an `ApiCoinSelection`. - change output derivation paths are valid.

Update integration test WALLETS_COIN_SELECTION_02.

ec674d8

Update integration test `WALLETS_COIN_SELECTION_02` to check that the set of coin selection outputs is exactly the same as intially provided, rather than being a subset.

Update integration test SHELLEY_HW_WALLETS/HW_WALLETS_04.

c32efdb

Update integration test `SHELLEY_HW_WALLETS/HW_WALLETS_04` to make multiple payments. In addition, we expect that the resulting selection will have: * an output for each of the original payments. * at least one change output.

Update integration test BYRON_HW_WALLETS/HW_WALLETS_04.

d237b3f

Update integration test `BYRON_HW_WALLETS/HW_WALLETS_04` to make multiple payments. In addition, we expect that the resulting selection will have: * an output for each of the original payments. * at least one change output.

Update integration test STAKE_POOLS_JOIN_UNSIGNED_01.

8067e52

We now expect that there are: * no ordinary outputs. * at least one change output.

Update integration test STAKE_POOLS_JOIN_UNSIGNED_02.

1c0911f

We now expect that there are: * no ordinary outputs. * at least one change output.

Update integration test STAKE_POOLS_QUIT_UNSIGNED_01.

ebcfd02

We now expect that there are: * no ordinary outputs. * at least one change output.

jonathanknowles force-pushed the jonathanknowles/change-outputs-derivation-paths branch from 5295989 to ebcfd02 Compare October 21, 2020 05:23

iohk-bors bot added a commit that referenced this pull request Oct 21, 2020

Try #2244:

c007bdd

iohk-bors bot merged commit f939d02 into master Oct 21, 2020

iohk-bors bot deleted the jonathanknowles/change-outputs-derivation-paths branch October 21, 2020 14:30

jonathanknowles mentioned this pull request Oct 22, 2020

Update the API specification to reflect the minimum number of inputs returned by selectCoins. #2261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add derivation paths to coin selection change outputs #2244

Add derivation paths to coin selection change outputs #2244

jonathanknowles commented Oct 15, 2020 •

edited

Loading

This comment has been minimized.

paweljakubas Oct 15, 2020

jonathanknowles Oct 15, 2020

This comment has been minimized.

iohk-bors bot commented Oct 16, 2020

paweljakubas left a comment

jonathanknowles commented Oct 21, 2020

jonathanknowles commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020 •

edited by Anviking

Loading

jonathanknowles commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

Field	Description
`outputs`	ordinary payments to external wallets
`change`	change to be returned to the wallet

Add derivation paths to coin selection change outputs #2244

Add derivation paths to coin selection change outputs #2244

Conversation

jonathanknowles commented Oct 15, 2020 • edited Loading

Issue Number

Overview

Internal Changes

Testing

This comment has been minimized.

paweljakubas Oct 15, 2020

Choose a reason for hiding this comment

jonathanknowles Oct 15, 2020

Choose a reason for hiding this comment

This comment has been minimized.

iohk-bors bot commented Oct 16, 2020

try

paweljakubas left a comment

Choose a reason for hiding this comment

jonathanknowles commented Oct 21, 2020

jonathanknowles commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

try

iohk-bors bot commented Oct 21, 2020 • edited by Anviking Loading

jonathanknowles commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

iohk-bors bot commented Oct 21, 2020

jonathanknowles commented Oct 15, 2020 •

edited

Loading

iohk-bors bot commented Oct 21, 2020 •

edited by Anviking

Loading