Add `constraints` field to `TransactionLayer` #2620

jonathanknowles · 2021-04-23T09:22:09Z

Issue Number

ADP-839

Overview

This PR:

adds field constraints of type TxConstraints to the TransactionLayer record type.
adds an implementation of constraints to Shelley.Transaction.TransactionLayer.
adds property tests to demonstrate the correctness of Shelley.Transaction.TransactionLayer.constraints.

Refactoring

This PR also:

makes TxSize into a concrete type exported by Primitive.Types.Tx.
removes calcMinimumCoinValue from TransactionLayer, as TxConstraints.txOutputMinimumAdaQuantity makes this redundant.

Future work

This PR will make it possible to:

remove tokenBundleSizeAssessor from TransactionLayer, as TxConstraints.txOutputMaximumSize makes this redundant.

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

Anviking

lgtm!

Anviking · 2021-04-28T13:41:55Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

@@ -540,45 +550,82 @@ _decodeSignedTx era bytes = do
        _ ->
            Left ErrDecodeSignedTxNotSupported

+data TxSkeleton = TxSkeleton


Might as well put the

his record type includes the minimal set of fields that are absolutely
necessary for the estimateTxSize function to perform its calculations,
and nothing else.

As such, the fields of TxSkeleton contain a subset of the data included
in the union of SelectionSkeleton and TransactionCtx.

From the commit message as a doc comment here.

Ah good point! I'll add some comments in there. 👍🏻

Fixed in d7ff662.

Anviking · 2021-04-28T14:08:49Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+-- between 'txOutputSize' and 'txOutputMaximumSize' should also indicate that
+-- the bundle is oversized.
+--
+prop_txConstraints_txOutputMaximumSize :: Large TokenBundle -> Property


Anviking · 2021-04-28T14:09:01Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+-- selection produces a result that is consistent with the result of using
+-- 'estimateTxCost'.
+--
+prop_txConstraints_txCost :: MockSelection -> Property


lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

jonathanknowles · 2021-04-29T01:21:03Z

bors r+

2620: Add `constraints` field to `TransactionLayer` r=jonathanknowles a=jonathanknowles # Issue Number ADP-839 # Overview This PR: - [x] adds field `constraints` of type `TxConstraints` to the `TransactionLayer` record type. - [x] adds an implementation of `constraints` to `Shelley.Transaction.TransactionLayer`. - [x] adds property tests to demonstrate the correctness of `Shelley.Transaction.TransactionLayer.constraints`. # Refactoring This PR also: - [x] makes `TxSize` into a concrete type exported by `Primitive.Types.Tx`. - [x] removes `calcMinimumCoinValue` from `TransactionLayer`, as `TxConstraints.txOutputMinimumAdaQuantity` makes this redundant. # Future work This PR will make it possible to: - remove `tokenBundleSizeAssessor` from `TransactionLayer`, as `TxConstraints.txOutputMaximumSize` makes this redundant. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-29T01:49:21Z

Build failed:

ci/hydra-build:required

Looks like this was a cache failure.

jonathanknowles · 2021-04-29T01:53:45Z

bors r+

2620: Add `constraints` field to `TransactionLayer` r=jonathanknowles a=jonathanknowles # Issue Number ADP-839 # Overview This PR: - [x] adds field `constraints` of type `TxConstraints` to the `TransactionLayer` record type. - [x] adds an implementation of `constraints` to `Shelley.Transaction.TransactionLayer`. - [x] adds property tests to demonstrate the correctness of `Shelley.Transaction.TransactionLayer.constraints`. # Refactoring This PR also: - [x] makes `TxSize` into a concrete type exported by `Primitive.Types.Tx`. - [x] removes `calcMinimumCoinValue` from `TransactionLayer`, as `TxConstraints.txOutputMinimumAdaQuantity` makes this redundant. # Future work This PR will make it possible to: - remove `tokenBundleSizeAssessor` from `TransactionLayer`, as `TxConstraints.txOutputMaximumSize` makes this redundant. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-04-29T02:00:10Z

Build failed:

ci/hydra-build:required

Looks like this was a cache failure.

After rerunning some of the tests, I ran into this failure: (https://hydra.iohk.io/build/6207105/nixlog/1/tail)

Failures:

 test/unit/Network/Wai/Middleware/LoggingSpec.hs:206:9:
 1) Network.Wai.Middleware.Logging, Logging Middleware, GET, 503
      uncaught exception: HttpException
      HttpExceptionRequest Request {
        host                 = "localhost"
        port                 = 54393
        secure               = False
        requestHeaders       = []
        path                 = "/error503"
        queryString          = ""
        method               = "GET"
        proxy                = Nothing
        rawBody              = False
        redirectCount        = 10
        responseTimeout      = ResponseTimeoutDefault
        requestVersion       = HTTP/1.1
      }
       ConnectionTimeout

 To rerun use: --match "/Network.Wai.Middleware.Logging/Logging Middleware/GET, 503/"

Randomized with seed 1415537019

Looks like a instance of:

#2368
#2217

jonathanknowles · 2021-04-29T12:29:15Z

bors r+

rvl · 2021-04-29T12:49:28Z

bors r-

Let's do the release first, then try to merge this.

iohk-bors · 2021-04-29T12:49:31Z

Canceled.

jonathanknowles · 2021-04-29T23:59:07Z

bors r+

2620: Add `constraints` field to `TransactionLayer` r=jonathanknowles a=jonathanknowles # Issue Number ADP-839 # Overview This PR: - [x] adds field `constraints` of type `TxConstraints` to the `TransactionLayer` record type. - [x] adds an implementation of `constraints` to `Shelley.Transaction.TransactionLayer`. - [x] adds property tests to demonstrate the correctness of `Shelley.Transaction.TransactionLayer.constraints`. # Refactoring This PR also: - [x] makes `TxSize` into a concrete type exported by `Primitive.Types.Tx`. - [x] removes `calcMinimumCoinValue` from `TransactionLayer`, as `TxConstraints.txOutputMinimumAdaQuantity` makes this redundant. # Future work This PR will make it possible to: - remove `tokenBundleSizeAssessor` from `TransactionLayer`, as `TxConstraints.txOutputMaximumSize` makes this redundant. Co-authored-by: Jonathan Knowles <[email protected]>

jonathanknowles · 2021-04-30T00:01:36Z

bors r-

iohk-bors · 2021-04-30T00:01:37Z

Canceled.

…ve.Tx`. Ultimately, transaction sizes are all expressible in terms of natural numbers, regardless of whether the sizes are expressed in terms of bytes, or in terms of some other unit. So there's currently little need for a type class abstraction here, when a newtype will suffice.

The purpose of `SelectionSkeleton` is to provide all the data necessary to calculate the fee associated with a selection. However, this calculation only depends on the number of inputs, and not the contents of each input. Therefore, it's only necessary to include a count of the selected inputs within `SelectionSkeleton`, and unnecessary to include the actual inputs themselves.

…on` prefix. This makes the record field names a bit more consistent.

This record type includes the minimal set of fields that are absolutely necessary for the `estimateTxSize` function to perform its calculations, and nothing else. As such, the fields of `TxSkeleton` contain a subset of the data included in the union of `SelectionSkeleton` and `TransactionCtx`. Using this type means that future tests for `estimateTxSize` will only need to supply values that will actually be used by the function, simplifying the effort to write tests.

This will allow it to be tested in conjunction with `txConstraints`.

The `constraints` field provides a value of `TxConstraints`.

Replace these uses with `constraints.txOutputMinimumAdaQuantity`.

This is made redundant by `constraints`.

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/2620/files#r622191033

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/2620/files#r622668819

We probably want to avoid spamming the test log with the output of `show` for very large token bundles. We already have a large set of counterexample data. In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/2620/files#r622669907

rvl · 2021-04-30T02:53:41Z

bors r+

iohk-bors · 2021-04-30T04:19:45Z

Build succeeded:

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from 356242e to 20c3255 Compare April 23, 2021 09:28

jonathanknowles requested a review from Anviking April 23, 2021 09:33

jonathanknowles changed the title ~~Add txConstraints field to TransactionLayer~~ Add constraints field to TransactionLayer Apr 23, 2021

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from 20c3255 to 1cf1668 Compare April 23, 2021 10:06

jonathanknowles self-assigned this Apr 23, 2021

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from 1cf1668 to 93eab53 Compare April 26, 2021 04:57

Anviking reviewed Apr 26, 2021

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Show resolved Hide resolved

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch 13 times, most recently from 36a01a8 to d98beeb Compare April 28, 2021 05:43

jonathanknowles marked this pull request as ready for review April 28, 2021 06:30

jonathanknowles requested a review from Anviking April 28, 2021 06:30

Anviking approved these changes Apr 28, 2021

View reviewed changes

jonathanknowles commented Apr 29, 2021

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved

jonathanknowles commented Apr 29, 2021

View reviewed changes

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated Show resolved Hide resolved

rvl approved these changes Apr 29, 2021

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from 2099f94 to c9470ca Compare April 29, 2021 12:27

jonathanknowles added 14 commits April 30, 2021 00:11

[Refactoring] Make fields of SelectionSkeleton share common `skelet…

b9478cb

…on` prefix. This makes the record field names a bit more consistent.

[Refactoring] Extract out function estimateTxCost.

0800fb4

This will allow it to be tested in conjunction with `txConstraints`.

Add constraints field to TransactionLayer.

c13845e

The `constraints` field provides a value of `TxConstraints`.

[Refactoring] Stop using calcMinimumCoinValue in Wallet.

fd3f276

Replace these uses with `constraints.txOutputMinimumAdaQuantity`.

[Refactoring] Remove calcMinimumCoinValue from TransactionLayer.

2ae580b

This is made redundant by `constraints`.

[Reformatting] Tidy function estimateTxSize.

d1af76d

Add basic property tests for txConstraints.

18d1a0a

Add property test for txOutputMaximumSize.

a31fa75

Add documentation comments for 'TxSkeleton' and related functions.

8f7b7b3

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/2620/files#r622191033

Use skeleton for arguments of type TxSkeleton.

04e9013

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/2620/files#r622668819

jonathanknowles force-pushed the jonathanknowles/multi-asset-migration-transaction-layer branch from c9470ca to e687560 Compare April 30, 2021 00:11

iohk-bors bot merged commit 182e864 into master Apr 30, 2021

iohk-bors bot deleted the jonathanknowles/multi-asset-migration-transaction-layer branch April 30, 2021 04:19

jonathanknowles mentioned this pull request Apr 30, 2021

API specification for multi-asset wallet migration #2590

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `constraints` field to `TransactionLayer` #2620

Add `constraints` field to `TransactionLayer` #2620

jonathanknowles commented Apr 23, 2021 •

edited

Loading

Anviking left a comment

Anviking Apr 28, 2021

jonathanknowles Apr 29, 2021

jonathanknowles Apr 29, 2021

Anviking Apr 28, 2021

Anviking Apr 28, 2021

jonathanknowles commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021 •

edited by jonathanknowles

Loading

jonathanknowles commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021 •

edited by jonathanknowles

Loading

jonathanknowles commented Apr 29, 2021

rvl commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021

jonathanknowles commented Apr 29, 2021

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

Add constraints field to TransactionLayer #2620

Add constraints field to TransactionLayer #2620

Conversation

jonathanknowles commented Apr 23, 2021 • edited Loading

Issue Number

Overview

Refactoring

Future work

Anviking left a comment

Choose a reason for hiding this comment

Anviking Apr 28, 2021

Choose a reason for hiding this comment

jonathanknowles Apr 29, 2021

Choose a reason for hiding this comment

jonathanknowles Apr 29, 2021

Choose a reason for hiding this comment

Anviking Apr 28, 2021

Choose a reason for hiding this comment

Anviking Apr 28, 2021

Choose a reason for hiding this comment

jonathanknowles commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021 • edited by jonathanknowles Loading

jonathanknowles commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021 • edited by jonathanknowles Loading

jonathanknowles commented Apr 29, 2021

rvl commented Apr 29, 2021

iohk-bors bot commented Apr 29, 2021

jonathanknowles commented Apr 29, 2021

jonathanknowles commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

rvl commented Apr 30, 2021

iohk-bors bot commented Apr 30, 2021

Add `constraints` field to `TransactionLayer` #2620

Add `constraints` field to `TransactionLayer` #2620

jonathanknowles commented Apr 23, 2021 •

edited

Loading

iohk-bors bot commented Apr 29, 2021 •

edited by jonathanknowles

Loading

iohk-bors bot commented Apr 29, 2021 •

edited by jonathanknowles

Loading