Withdrawals as part of core and API transactions #1892

KtorZ · 2020-07-09T15:32:38Z

Issue Number

Overview

74abe1c
📍 add withdrawals to API & core transaction data types
Still to be done: withdrawals must be discovered when processing blocks
0211c5c
📍 upgrade Byron & Jormungandr compatibility code with empty withdrawals
Withdrawals are only a thing in Shelley, so for this targets, we can safely replace them by empty lists
861a792
📍 wire withdrawals in Shelley compatibility code
Here, we extract withdrawals directly from the TxBody, the conversion is straightforward
c36f23a
📍 include withdrawals where relevant in test and arbitrary generators
9f6738e
📍 add withdrawals to the API Swagger specification
We represent them as a list and not a key:value map with stake address as key because clients typically don't know the stake address associated with their wallet, so it'd be quite impractical for them to access this data if returned as a map...
e7fcedb
📍 add roundtrip tests for stake address and show that withdrawals are returned from the API
00c2d69
📍 Add stake address constraints to Jormungandr and Byron
533b772
📍 add 'minWithdrawal' query parameter to the swagger specification
07a0f23
📍 extend transaction API to allow filtering by minimal withdrawals amount
This is a 'any' sort of filter, such that it returns any transaction where there's at least one withdrawal matching the criteria. The use-case is typically to ask for all transactions where there's a min withdrawal of 1, such that we get the entire withdrawal history
3dd7257
📍 review integration and unit tests, add additional checks to cover withdrawals

Comments

⚠️ Sorry, the PR spans over many files because I needed to add a new class constraints DecodeStakeAddress which goes side-by-side with the current DecodeAddress.

KtorZ · 2020-07-09T15:34:36Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+                _ <- Sqlite.step query
+                Sqlite.finalize query
+            ColumnPresent ->
+                traceWith tr $ MsgManualMigrationNotNeeded field


I originally started adding the withdrawal to the TxMeta table but I reverted and went for a new table instead since it behaves much more like current TxIn and TxOut. I however kept this refactor as I thought it wouldn't hurt 😊

KtorZ · 2020-07-09T15:36:33Z

lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs

+    txWithdrawalAccount W.ChimericAccount   sql=account
+
+    Primary txWithdrawalTxId txWithdrawalAccount
+    deriving Show Generic


Interesting stuff actually here ☝️

KtorZ · 2020-07-09T15:38:07Z

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

+    , withdrawals
+        :: !(Map ChimericAccount Coin)
+        -- ^ Withdrawals (of funds from a registered reward account) embedded in
+        -- a transaction. The order does not matter.


And here 👆

KtorZ · 2020-07-09T15:39:01Z

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

+                Right a
+      where
+        msg = TextDecodingError
+            "Unable to decode stake-address: not a well-formed address."


I'll add a roundtrip test for these instances.

KtorZ · 2020-07-09T15:41:00Z

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

@@ -911,9 +983,6 @@ instance Buildable addr => Buildable (ConnectionId addr) where
 instance Buildable LocalAddress where
    build (LocalAddress p) = build p

-instance Buildable W.ChimericAccount where
-    build (W.ChimericAccount addr) = hexF addr


Found this little orphan 🙃 ... No longer needed since it's define in the core package next to its relevant type declaration.

KtorZ · 2020-07-09T15:42:59Z

specifications/api/swagger.yaml

+      - amount
+    properties:
+      stake_address: *stakeAddress
+      amount: *amount


Here, I've chosen not to represent withdrawals as a key:value map in the API because it would otherwise be a key:value map where all keys are pretty much unknown by the client 😶 ... wallet clients aren't typically aware of what their stake address is, so having something like:

{ "stake_addr1sjck9mdmfyhzvjhydcjllgj9vjvl522w0573ncustrrr2rg7h9azg4cyqd36yyd48t5ut72hgld0fg2x": 42 }

Would make it quite hard for them to inspect the content of that value ^^"... Hence, in the API, I am turning the Map we have internally into a list of object with a stake_address and amount properties.

yeah, sound decision

Was thinking before how absent withdrawals are going to be represented: either by 0-length array or by making withdrawals in ApiTransaction not required... Why the first option is better?

It'll be a 0-length array.

paweljakubas · 2020-07-09T16:08:43Z

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

    , status :: !(ApiT TxStatus)
    } deriving (Eq, Generic, Show)

+data ApiWithdrawal n = ApiWithdrawal


paweljakubas · 2020-07-09T16:15:19Z

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

    , mapMaybe fromShelleyDelegationCert (toList certs)
    , mapMaybe fromShelleyRegistrationCert (toList certs)
    )

+fromShelleyWdrl :: SL.Wdrl TPraosStandardCrypto -> Map W.ChimericAccount W.Coin


paweljakubas · 2020-07-09T16:17:10Z

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

@@ -387,15 +387,15 @@ computeTxSize proxy action cs =

    dummyKeyHashRaw = BS.pack (replicate 28 0)

-    withdrawals = mkWithdrawals
+    wdrls = mkWithdrawals


frankly speaking like more previous naming, but understand the probable rationale (80-line styling)

Yeah, I usually try to keep naming consistent with the surrounding context.

paweljakubas

LGTM! would be good not to forget _decodeStakeAddress . _encodeStakeAddress roundtrip. Also some changes are needed in /build/cardano-wallet/lib/core/test/bench/db/Main.hs:408 but it is no surprise when changes in such a core place are made.

KtorZ · 2020-07-10T13:48:01Z

bors r+

iohk-bors · 2020-07-10T13:50:39Z

Canceled

KtorZ · 2020-07-10T13:50:57Z

bors r+

1892: Withdrawals as part of core and API transactions r=KtorZ a=KtorZ # Issue Number  #1881 # Overview  - 589609e 📍 **factor out common code in database migration** Turns out that adding column is a pretty common and straighfordward migration. I originally intended to add a new column to the TxMeta table, hence the refactor. In the end, I've used a different table for withdrawals, but the refactor is still worth it as it is IMO. - e9e516c 📍 **add withdrawals to API & core transaction data types** Still to be done: withdrawals must be discovered when processing blocks - ff58489 📍 **upgrade Byron & Jormungandr compatibility code with empty withdrawals** Withdrawals are only a thing in Shelley, so for this targets, we can safely replace them by empty lists - 533b787 📍 **wire withdrawals in Shelley compatibility code** Here, we extract withdrawals directly from the TxBody, the conversion is straightforward - e2e6fbb 📍 **include withdrawals where relevant in test and arbitrary generators** - 4b6eaba 📍 **add withdrawals to the API Swagger specification** We represent them as a list and not a key:value map with stake address as key because clients typically don't know the stake address associated with their wallet, so it'd be quite impractical for them to access this data if returned as a map... # Comments  ⚠️ Sorry, the PR spans over many files because I needed to add a new class constraints `DecodeStakeAddress` which goes side-by-side with the current `DecodeAddress`.  1895: hydra: Prevent caching of shelley integration test failures r=KtorZ a=rvl ### Issue Number Not sure. ### Overview This change causes integration tests to be re-run whenever the git revision changes, even if everything else is identical. Since these tests tend to fail a lot, we don't want to cache false failures. Also increase the minimum severity for integration test logging, because debug level produces quite a lot of output. Co-authored-by: KtorZ <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2020-07-10T14:55:44Z

Build failed (retrying...)

buildkite/cardano-wallet

iohk-bors · 2020-07-10T15:01:58Z

Canceled

Still to be done: withdrawals must be discovered when processing blocks

Withdrawals are only a thing in Shelley, so for this targets, we can safely replace them by empty lists

Here, we extract withdrawals directly from the TxBody, the conversion is straightforward

We represent them as a list and not a key:value map with stake address as key because clients typically don't know the stake address associated with their wallet, so it'd be quite impractical for them to access this data if returned as a map...

…eturned from the API

This is a 'any' sort of filter, such that it returns any transaction where there's at least one withdrawal matching the criteria. The use-case is typically to ask for all transactions where there's a min withdrawal of 1, such that we get the entire withdrawal history

…hdrawals

KtorZ · 2020-07-10T21:52:58Z

bors r+

1892: Withdrawals as part of core and API transactions r=KtorZ a=KtorZ # Issue Number  #1881 # Overview  - 74abe1c 📍 **add withdrawals to API & core transaction data types** Still to be done: withdrawals must be discovered when processing blocks - 0211c5c 📍 **upgrade Byron & Jormungandr compatibility code with empty withdrawals** Withdrawals are only a thing in Shelley, so for this targets, we can safely replace them by empty lists - 861a792 📍 **wire withdrawals in Shelley compatibility code** Here, we extract withdrawals directly from the TxBody, the conversion is straightforward - c36f23a 📍 **include withdrawals where relevant in test and arbitrary generators** - 9f6738e 📍 **add withdrawals to the API Swagger specification** We represent them as a list and not a key:value map with stake address as key because clients typically don't know the stake address associated with their wallet, so it'd be quite impractical for them to access this data if returned as a map... - e7fcedb 📍 **add roundtrip tests for stake address and show that withdrawals are returned from the API** - 00c2d69 📍 **Add stake address constraints to Jormungandr and Byron** - 533b772 📍 **add 'minWithdrawal' query parameter to the swagger specification** - 07a0f23 📍 **extend transaction API to allow filtering by minimal withdrawals amount** This is a 'any' sort of filter, such that it returns any transaction where there's at least one withdrawal matching the criteria. The use-case is typically to ask for all transactions where there's a min withdrawal of 1, such that we get the entire withdrawal history - 3dd7257 📍 **review integration and unit tests, add additional checks to cover withdrawals** # Comments  ⚠️ Sorry, the PR spans over many files because I needed to add a new class constraints `DecodeStakeAddress` which goes side-by-side with the current `DecodeAddress`.  Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-07-10T22:04:07Z

Canceled

KtorZ · 2020-07-10T22:05:24Z

bors r+

iohk-bors · 2020-07-10T23:03:20Z

Build succeeded

KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jul 9, 2020

KtorZ requested a review from paweljakubas July 9, 2020 15:32

KtorZ self-assigned this Jul 9, 2020

KtorZ commented Jul 9, 2020

View reviewed changes

paweljakubas reviewed Jul 9, 2020

View reviewed changes

paweljakubas approved these changes Jul 9, 2020

View reviewed changes

KtorZ force-pushed the KtorZ/1881/withdrawals-in-transactions branch from a64f033 to a40b93e Compare July 10, 2020 13:37

KtorZ force-pushed the KtorZ/1881/withdrawals-in-transactions branch from ab7bb2b to c947ed4 Compare July 10, 2020 15:01

KtorZ force-pushed the KtorZ/1881/withdrawals-in-transactions branch 2 times, most recently from c947ed4 to d344780 Compare July 10, 2020 20:06

KtorZ added 7 commits July 10, 2020 23:50

add withdrawals to API & core transaction data types

74abe1c

Still to be done: withdrawals must be discovered when processing blocks

upgrade Byron & Jormungandr compatibility code with empty withdrawals

0211c5c

Withdrawals are only a thing in Shelley, so for this targets, we can safely replace them by empty lists

wire withdrawals in Shelley compatibility code

861a792

Here, we extract withdrawals directly from the TxBody, the conversion is straightforward

include withdrawals where relevant in test and arbitrary generators

c36f23a

add roundtrip tests for stake address and show that withdrawals are r…

e7fcedb

…eturned from the API

Add stake address constraints to Jormungandr and Byron

00c2d69

KtorZ added 3 commits July 10, 2020 23:50

add 'minWithdrawal' query parameter to the swagger specification

533b772

review integration and unit tests, add additional checks to cover wit…

3dd7257

…hdrawals

KtorZ force-pushed the KtorZ/1881/withdrawals-in-transactions branch from 675d507 to 3dd7257 Compare July 10, 2020 21:50

Regenerate nix

26064b2

This was referenced Jul 10, 2020

Add withdrawals to core transaction data-type #1881

Closed

Allow filtering txs by minimal withdrawal amount in the API #1882

Closed

iohk-bors bot merged commit df141f5 into master Jul 10, 2020

iohk-bors bot deleted the KtorZ/1881/withdrawals-in-transactions branch July 10, 2020 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Withdrawals as part of core and API transactions #1892

Withdrawals as part of core and API transactions #1892

KtorZ commented Jul 9, 2020 •

edited

Loading

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

paweljakubas Jul 9, 2020

paweljakubas Jul 9, 2020 •

edited

Loading

KtorZ Jul 9, 2020

paweljakubas Jul 9, 2020

paweljakubas Jul 9, 2020

paweljakubas Jul 9, 2020

KtorZ Jul 9, 2020

paweljakubas left a comment •

edited

Loading

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Withdrawals as part of core and API transactions #1892

Withdrawals as part of core and API transactions #1892

Conversation

KtorZ commented Jul 9, 2020 • edited Loading

Issue Number

Overview

Comments

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

paweljakubas Jul 9, 2020 • edited Loading

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

paweljakubas left a comment • edited Loading

Choose a reason for hiding this comment

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Canceled

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Build failed (retrying...)

iohk-bors bot commented Jul 10, 2020

Canceled

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Canceled

KtorZ commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Build succeeded

KtorZ commented Jul 9, 2020 •

edited

Loading

paweljakubas Jul 9, 2020 •

edited

Loading

paweljakubas left a comment •

edited

Loading